Bug 32407

Created attachment 51545 [details] [review]
Include-selinux-code-only-when-enabled-autotools-part

Created attachment 51546 [details] [review]
Include-selinux-code-only-when-enabled-cmake-parts

Created attachment 51547 [details] [review]
Include-selinux-test-data-only-when-enabled

Created attachment 51548 [details] [review]
Include-selinux-code-only-when-enabled.patch

Created attachment 51549 [details] [review]
include-selinux-stuff-in-session.conf

It was probably a mistake for the SELinux methods to be part of the org.freedesktop.DBus interface (they should have been on a conditionally-present org.freedesktop.SELinux interface), but now that they are, I think they should be implemented (and just raise some sort of NotImplemented error) on all platforms: a D-Bus interface is meant to be, conceptually, a collection of methods/etc. where if you have one, you have them all.

(Surely the SELinux-related methods are already stubs on non-Linux systems anyway?)

I think that having stub implementations that do nothing in the absence of SELinux is a lot easier to follow than this sort of #ifdef soup.

> +if DBUS_HAVE_SELINUX
> +DBUS_CONF_INCLUDE_SELINUX =
> +    <include if_selinux_enabled="yes"
> selinux_root_relative="yes">contexts/dbus_contexts</include>
> +endif

This configuration is already ignored on non-SELinux platforms. You can tell by how it says 'if_selinux_enabled="yes"'...

> Subject: [PATCH 4/5] Include selinux code only when enabled

I'd rather not do this:

> +#ifdef HAVE_SELINUX
> +    if (!bus_selinux_full_init ())

I'd prefer to keep bus_selinux_full_init() doing nothing, successfully, on platforms without it - otherwise, you're trading CPU time (which is cheap) for D-Bus maintainer time (which is a limited resource) by adding to the complexity of working out what happens when. If you particularly want to make it inlinable, in order to tell your compiler that it can assume it never fails, the hacks for that should go in selinux.h (#define bus_selinux_full_init() (TRUE) on non-SELinux platforms, or whatever), but I think that'd be a premature optimization (unless you have profiling data that demonstrates otherwise).

> +#ifdef HAVE_SELINUX
>  	if (e->d.include.if_selinux_enabled
>  	    && !bus_selinux_enabled ())
>  	  break;
> -
> +#endif

The entire point of the if_selinux_enabled attribute is to make non-SELinux dbus-daemons ignore those bits of configuration; again, if you like premature optimization, you could make bus_selinux_enabled a function-like macro that expands to TRUE, which would make competent compilers delete the conditional entirely.

+if HAVE_SELINUX
+DIR_SELINUX_SOURCE=selinux.c selinux.h
+else
+DIR_SELINUX_SOURCE=
+endif

I strongly prefer not to do this sort of thing: I think having the source code of selinux.c compile down to nothing, or a file full of trivial stub implementations, on non-SELinux platforms is a better solution (particularly since that puts the conditional in selinux.c, which there's only one of, rather than duplicating it in the cmake and autoconf build systems). See stats.[ch], for instance (only on master; absent on dbus-1.4).

(In reply to comment #0)
> In dbus-daemons build on windows there are unrelated methods visible  
> 
> >qdbus org.freedesktop.DBus /
> method QByteArray
> org.freedesktop.DBus.GetConnectionSELinuxSecurityContext(QString)
> 
> These functions should be removed.

I don't think they should be removed (whether we like it or not, they're part of the o.fd.DBus interface), but equally, I don't think we should add any more. Bug #54445 provides an alternative way to support arbitrary credentials frameworks, including Windows SIDs, without endless proliferation of methods.

Resolving. WONTFIX is the closest we have to "sorry, past maintainers were wrong to put these on a core interface, but it won't happen again". :-)