Bug 77032

(In reply to comment #0)
> The flag is not passed in the dbus version currently in Debian/Testing
> (1.6.18)

That was a bug (Bug #72213). The fd should be set close-on-exec somehow; in modern OSs, O_CLOEXEC is the preferred way to do that, but Mac OS 10.6 appears to be based on an older FreeBSD version.

> presuming that this approach is still viable in v1.8.0 it would be
> nice if the build system checked whether the C_CLOEXEC is defined or not. A
> better workaround would of course be preferable.

I suggested a solution in <https://bugs.freedesktop.org/show_bug.cgi?id=72213#c2>. I'd be happy to review a patch that did something similar.

D-Bus' primary target OS is Linux, followed by Windows and *BSD; there is no active maintainer who uses Mac OS. If people would like it to work better on Mac OS, testing and patches are welcome.

If the solution you suggested looks like this

-          fd = open (new_dirs[i], O_RDONLY);
+#ifdef O_CLOEXEC
+          fd = open (new_dirs[i], O_RDONLY | O_CLOEXEC);
+
+          if (retval < 0 && errno == EINVAL)
+#endif
+            fd = open (new_dirs[i], O_RDONLY);
+
+          _dbus_fd_set_close_on_exec (fd);

then it ought to do the trick for OS X 10.6 . I've been using the dbus daemon without O_CLOEXEC since I reported the issue, without any apparent problems.

(As to testing on Mac OS X: I don't recall seeing complaints about dbus not working properly on the MacPorts mailing list. Without knowing exactly how intensively it gets used in that environment it's at least an indicator that it's serving its purpose, right? :) )

(In reply to comment #2)
> If the solution you suggested looks like this
> 
> -          fd = open (new_dirs[i], O_RDONLY);
> +#ifdef O_CLOEXEC
> +          fd = open (new_dirs[i], O_RDONLY | O_CLOEXEC);
> +
> +          if (retval < 0 && errno == EINVAL)
> +#endif
> +            fd = open (new_dirs[i], O_RDONLY);
> +
> +          _dbus_fd_set_close_on_exec (fd);
> 
> then it ought to do the trick for OS X 10.6 .

Please try that, or removing O_CLOEXEC and adding _dbus_fd_set_close_on_exec (fd), or something, and report back with a tested patch; I can't test on Mac OS, but I'm happy to apply patches from people who can.

> I've been using the dbus
> daemon without O_CLOEXEC since I reported the issue, without any apparent
> problems.

The problem with not having close-on-exec is that when dbus-daemon executes a subprocess (an activatable D-Bus service), that subprocess will inherit a reference to the kqueue fd (as a file descriptor > 2), which could cause either the subprocess or dbus-daemon to misbehave.

On Linux you'd be able to see that by looking in /proc/$pid/fd/ where $pid is the process ID of an activated service. I don't know whether Mac OS has an equivalent.

(In reply to comment #3)

> Please try [that, or] removing O_CLOEXEC and adding _dbus_fd_set_close_on_exec
> (fd), or something, and report back with a tested patch; I can't test on Mac
> OS, but I'm happy to apply patches from people who can.
> 

That is the solution that has been tested by MacPorts, and that works. I'm adding a more elaborate patch that's transparent on systems that do have O_CLOEXEC (i.e. it follows the suggestion above). As far as I can see it tests out fine.

Created attachment 98300 [details] [review]
patch to use alternative code path on systems lacking O_CLOEXEC

(In reply to comment #4)
> That is the solution that has been tested by MacPorts, and that works. I'm

Note that a different solution is also discussed on MacPorts trac: https://trac.macports.org/ticket/43203#comment:1
it involves using the O_CLOEXEC functionality provided by Gnulib.

Comment on attachment 98300 [details] [review]
patch to use alternative code path on systems lacking O_CLOEXEC

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

::: bus/dir-watch-kqueue.c.orig
@@ +263,4 @@
>            fd = open (new_dirs[i], O_RDONLY | O_CLOEXEC);
> +#else
> +          fd = open (new_dirs[i], O_RDONLY);
> +#endif

Unfortunately this isn't going to work if your libc supports O_CLOEXEC, but your kernel doesn't. I don't know how common this is on *BSD, but it's a relatively common situation on GNU systems, where the kernel and glibc are maintained separately. That's why the implementation I suggested on the other bug is a bit more complex, and tests for EINVAL.

@@ +279,5 @@
>                  }
>              }
> +#ifndef O_CLOEXEC
> +		_dbus_fd_set_close_on_exec(fd);
> +#endif

Just removing the use of O_CLOEXEC, and adding this line without the #ifdefs (and with more normal indentation), would probably be the simplest way to fix this - the fd would briefly be open-but-not-O_CLOEXEC, but this module isn't used in threaded code anyway, so it doesn't actually matter.

(In reply to comment #6)
> Note that a different solution is also discussed on MacPorts trac:
> https://trac.macports.org/ticket/43203#comment:1
> it involves using the O_CLOEXEC functionality provided by Gnulib.

I'd rather not copy in files from gnulib unless it's absolutely vital that we do so.

NetBSD-5.x is still supported release, but lacking O_CLOEXEC.

using following patch:

--- bus/dir-watch-kqueue.c.orig 2014-01-06 16:02:19.000000000 +0000
+++ bus/dir-watch-kqueue.c
@@ -259,7 +259,15 @@ bus_set_watched_dirs (BusContext *contex
           /* FIXME - less lame error handling for failing to add a watch;
            * we may need to sleep.
            */
+#ifdef O_CLOEXEC
           fd = open (new_dirs[i], O_RDONLY | O_CLOEXEC);
+
+          if (fd < 0 && errno == EINVAL)
+#endif
+          {
+            fd = open (new_dirs[i], O_RDONLY);
+            _dbus_fd_set_close_on_exec (fd);
+          }
           if (fd < 0)
             {
               if (errno != ENOENT)

(In reply to comment #9)

Almost there, but:

(In reply to comment #9)
> +            fd = open (new_dirs[i], O_RDONLY);
> +            _dbus_fd_set_close_on_exec (fd);

Calling _dbus_fd_set_close_on_exec (-1) is undefined behaviour. That function call should move further down, after (fd < 0) has been checked.

When the fd is >= 0, a redundant call to _dbus_fd_set_close_on_exec() would be OK even if the fd is already O_CLOEXEC - it only costs one unnecessary syscall per run of dbus-daemon.

If possible please attach "git format-patch" output so we can attribute the patch to you properly. (If not, I'll make up a suitable commit message, and use the name and email address of your Bugzilla account for attribution.)

Created attachment 100518 [details] [review]
portability patch for systems which lack O_CLOEXEC

This patch gives O_CLOEXEC the same treatment we give to SOCK_CLOEXEC elsewhere.

Comment on attachment 98300 [details] [review]
patch to use alternative code path on systems lacking O_CLOEXEC

obsoleted by Patrick's patch (I'll mention all contributors in NEWS)

Comment on attachment 100518 [details] [review]
portability patch for systems which lack O_CLOEXEC

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

Looks fine, I'll apply it next time I do a batch of work on 1.9.x.

Fixed in git for 1.9.0, suitable for backporting to dbus-1.8 by ports/packagers