Bug 100912 - Remove deprecated (and noop) calls to sidget/sidput
Summary: Remove deprecated (and noop) calls to sidget/sidput
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-03 10:01 UTC by Laurent Bigonville
Modified: 2017-05-31 13:10 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
0001-Remove-calls-to-sidget-sidput.patch (3.82 KB, patch)
2017-05-03 15:26 UTC, Laurent Bigonville
Details | Splinter Review
0002-Do-not-transtype-bus_sid-in-avc_has_perm-call.patch (862 bytes, patch)
2017-05-03 15:27 UTC, Laurent Bigonville
Details | Splinter Review
0003-configure.ac-Drop-check-for-selinux-av_permissions.h.patch (1.37 KB, patch)
2017-05-04 12:38 UTC, Laurent Bigonville
Details | Splinter Review
0004-Use-pkg-config-to-detect-libselinux-and-force-versio.patch (1.29 KB, patch)
2017-05-04 12:38 UTC, Laurent Bigonville
Details | Splinter Review
0002-Remove-unnecessary-cast-bus_sid-is-already-of-type-s.patch (842 bytes, patch)
2017-05-04 12:47 UTC, Laurent Bigonville
Details | Splinter Review
0004-Use-pkg-config-to-detect-libselinux-and-force-versio.patch (1.68 KB, patch)
2017-05-04 12:47 UTC, Laurent Bigonville
Details | Splinter Review
0001-Remove-calls-to-sidget-sidput.patch (5.25 KB, patch)
2017-05-04 13:15 UTC, Laurent Bigonville
Details | Splinter Review
0003-configure.ac-Drop-check-for-selinux-av_permissions.h.patch (1.44 KB, patch)
2017-05-04 13:16 UTC, Laurent Bigonville
Details | Splinter Review

Description Laurent Bigonville 2017-05-03 10:01:17 UTC
Hi,

dbus-daemon is calling sidget/sidput functions which are deprecated and noop since libselinux 2.0.86 (2009).

I guess that the calls to these functions can be removed

I apparently wrote a patch for this a while back, I'll check if it is still OK and propose it here.
Comment 1 Simon McVittie 2017-05-03 11:00:29 UTC
Please ask someone who understands SELinux to ack any changes here - I don't know how it works other than "it's security-sensitive so be careful".
Comment 2 Philip Withnall 2017-05-03 11:34:04 UTC
(In reply to Simon McVittie from comment #1)
> Please ask someone who understands SELinux to ack any changes here - I don't
> know how it works other than "it's security-sensitive so be careful".

`man sidget` says:
> As of libselinux version 2.0.86, SID's are no longer reference counted.
> A  SID  will be valid from the time it is first obtained until the next
> call to avc_destroy(3).  The sidget(3) and  sidput(3)  functions,  for‐
> merly  used  to  adjust  the reference count, are no-ops and are depre‐
> cated.
Comment 3 Laurent Bigonville 2017-05-03 15:26:54 UTC
Created attachment 131186 [details] [review]
0001-Remove-calls-to-sidget-sidput.patch
Comment 4 Laurent Bigonville 2017-05-03 15:27:14 UTC
Created attachment 131187 [details] [review]
0002-Do-not-transtype-bus_sid-in-avc_has_perm-call.patch
Comment 5 Philip Withnall 2017-05-04 11:37:29 UTC
Comment on attachment 131186 [details] [review]
0001-Remove-calls-to-sidget-sidput.patch

Review of attachment 131186 [details] [review]:
-----------------------------------------------------------------

This should probably also include changes to configure.ac to bump our libselinux dependency to ≥ 2.0.86.
Comment 6 Philip Withnall 2017-05-04 11:38:23 UTC
Comment on attachment 131187 [details] [review]
0002-Do-not-transtype-bus_sid-in-avc_has_perm-call.patch

Review of attachment 131187 [details] [review]:
-----------------------------------------------------------------

This looks correct. r+ from me.
Comment 7 Simon McVittie 2017-05-04 12:14:48 UTC
(In reply to Philip Withnall from comment #5)
> This should probably also include changes to configure.ac to bump our
> libselinux dependency to ≥ 2.0.86.

I agree. We should also be careful to call this out in NEWS (search for "Dependencies:" to see the typical convention).
Comment 8 Simon McVittie 2017-05-04 12:17:20 UTC
(In reply to Philip Withnall from comment #6)
> This looks correct. r+ from me.

Looks correct, but I'd prefer a commit message that looks more like "remove unnecessary cast" - transtype is not common jargon (at least in libdbus).
Comment 9 Laurent Bigonville 2017-05-04 12:38:19 UTC
Created attachment 131200 [details] [review]
0003-configure.ac-Drop-check-for-selinux-av_permissions.h.patch
Comment 10 Laurent Bigonville 2017-05-04 12:38:34 UTC
Created attachment 131201 [details] [review]
0004-Use-pkg-config-to-detect-libselinux-and-force-versio.patch
Comment 11 Laurent Bigonville 2017-05-04 12:47:04 UTC
Created attachment 131202 [details] [review]
0002-Remove-unnecessary-cast-bus_sid-is-already-of-type-s.patch
Comment 12 Laurent Bigonville 2017-05-04 12:47:24 UTC
Created attachment 131203 [details] [review]
0004-Use-pkg-config-to-detect-libselinux-and-force-versio.patch
Comment 13 Philip Withnall 2017-05-04 12:56:13 UTC
Comment on attachment 131200 [details] [review]
0003-configure.ac-Drop-check-for-selinux-av_permissions.h.patch

Review of attachment 131200 [details] [review]:
-----------------------------------------------------------------

Would be worthwhile mentioning that (as far as I can tell), the last use of av_permissions.h in D-Bus was dropped in commit ba088208bc0c35ca418a097a8482c4a7705f4a43, in 2013.
Comment 14 Philip Withnall 2017-05-04 12:57:08 UTC
Comment on attachment 131203 [details] [review]
0004-Use-pkg-config-to-detect-libselinux-and-force-versio.patch

Review of attachment 131203 [details] [review]:
-----------------------------------------------------------------

This should be squashed into the commit which drops sidget()/sidput().
Comment 15 Laurent Bigonville 2017-05-04 13:15:44 UTC
Created attachment 131205 [details] [review]
0001-Remove-calls-to-sidget-sidput.patch
Comment 16 Laurent Bigonville 2017-05-04 13:16:30 UTC
Created attachment 131206 [details] [review]
0003-configure.ac-Drop-check-for-selinux-av_permissions.h.patch
Comment 17 Simon McVittie 2017-05-05 16:53:47 UTC
Comment on attachment 131202 [details] [review]
0002-Remove-unnecessary-cast-bus_sid-is-already-of-type-s.patch

Review of attachment 131202 [details] [review]:
-----------------------------------------------------------------

Looks good
Comment 18 Simon McVittie 2017-05-05 16:54:58 UTC
Comment on attachment 131205 [details] [review]
0001-Remove-calls-to-sidget-sidput.patch

Review of attachment 131205 [details] [review]:
-----------------------------------------------------------------

Looks OK
Comment 19 Simon McVittie 2017-05-05 16:55:24 UTC
Comment on attachment 131206 [details] [review]
0003-configure.ac-Drop-check-for-selinux-av_permissions.h.patch

Review of attachment 131206 [details] [review]:
-----------------------------------------------------------------

Looks good. I like code deletion!
Comment 20 Simon McVittie 2017-05-31 13:10:09 UTC
Thanks, fixed in git for 1.11.14.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.