In Debian bug <https://bugs.debian.org/829348>, lightdm appears to have been starting dbus-launch with at least one of the three standard fds 0, 1, 2 (stdin, stdout, stderr) closed. This resulted in the dbus-daemon's epoll_create1() returning a fd less than 3. Later, _dbus_become_daemon() replaces fds 0-2 with /dev/null. As a result, a subsequent call to _dbus_loop_add_watch() for the reload pipe resulted in calling epoll_ctl on the non-epoll fd pointing to /dev/null, which fails with EINVAL, resulting in the dbus-daemon exiting unsuccessfully. Unix programs are not normally expected to behave correctly when launched with the standard fds not already open; but at the same time, X11 autolaunching means that dbus-launch (and hence the dbus-daemon) can get started from an arbitrarily precarious situation. So we should defend against this. This is the sort of Unix magic that Thiago and Colin are particularly good at, so I would particularly appreciate review from one of them.
Created attachment 125161 [details] [review] test: add a regression test for eval "$(dbus-launch --sh-syntax)"
Created attachment 125162 [details] [review] Add a simple test for dbus-daemon --fork
Created attachment 125163 [details] [review] _dbus_open_standard_fds: new function to ensure std* fds are open This function opens stdin, stdout, stderr pointing to /dev/null if they aren't already open. Optionally, it can also replace whatever is available on those fds with /dev/null. To allow for use in contexts where only async-signal-safe functions should be used, such as between fork() and a following exec(), this function does not use conventional libdbus error handling (which would require malloc). Instead, it sets errno and returns an explanatory string. --- Also proposed for 1.10, to fix the bug.
Created attachment 125164 [details] [review] dbus-daemon, dbus-launch: cope with callers having closed standard fds In Debian bug <https://bugs.debian.org/829348>, lightdm appears to have been starting dbus-launch with at least one of the three standard fds 0, 1, 2 (stdin, stdout, stderr) closed. This resulted in the dbus-daemon's epoll_create1() returning a fd less than 3. Later, _dbus_become_daemon() replaces fds 0-2 with /dev/null. As a result, a subsequent call to _dbus_loop_add_watch() for the reload pipe resulted in calling epoll_ctl on the non-epoll fd pointing to /dev/null, which fails with EINVAL, resulting in the dbus-daemon exiting unsuccessfully. Unix programs are not normally expected to behave correctly when launched with the standard fds not already open; but at the same time, X11 autolaunching means that dbus-launch (and hence the dbus-daemon) can get started from an arbitrarily precarious situation. --- Also proposed for 1.10. This actually fixes the bug.
Created attachment 125165 [details] [review] _dbus_become_daemon: use _dbus_open_standard_fds, and report errors --- Non-essential, so probably best for 1.11 only.
Created attachment 125166 [details] [review] dbus-launch: use _dbus_open_standard_fds() in the babysitter --- 1.11 only.
Created attachment 125167 [details] [review] dbus-launch: use _dbus_open_standard_fds when closing stderr --- 1.11 only.
Created attachment 125168 [details] [review] _read_subprocess_line_argv: use _dbus_open_standard_fds This also gives us an opportunity to improve the error reporting. --- 1.11.
Created attachment 125169 [details] [review] test: expand dbus-launch-eval test to cover stdin being closed
Created attachment 125170 [details] [review] test: expand dbus-launch-eval test to cover stdin being closed
The tests are self-contained (they don't mess with anything non-test), so they could go to the stable branch too, or not if people would prefer to keep the diffstat down.
Which order do the patches go?
Comment on attachment 125161 [details] [review] test: add a regression test for eval "$(dbus-launch --sh-syntax)" Review of attachment 125161 [details] [review]: ----------------------------------------------------------------- Not familiar with the test infra. ::: test/test-dbus-launch-eval.sh @@ +1,3 @@ > +#!/bin/sh > + > +# Copyright © 2016 Collabora Ltd. Splinter can't display UTF-8? WTF? We're in 2016....
Comment on attachment 125163 [details] [review] _dbus_open_standard_fds: new function to ensure std* fds are open Review of attachment 125163 [details] [review]: ----------------------------------------------------------------- Code looks good. Suggestions are to make it easier to understand, as it took me a while to understand the behaviour of the flags. ::: dbus/dbus-sysdeps-unix.c @@ +147,5 @@ > + * This function can only be called while single-threaded: either during > + * startup of an executable, or after fork(). > + */ > +dbus_bool_t > +_dbus_open_standard_fds (DBusOpenStandardFdsFlags flags, I'd name it "ensure" instead of "open". If you passed 0 for flags, it wouldn't open anything. @@ +177,5 @@ > + goto out; > + } > + > + /* We already opened all fds < i. */ > + _dbus_assert (devnull >= i); This can only fail if another thread closed an fd that we just opened. That's brokenness on the other thread, so ok here. ::: dbus/dbus-sysdeps-unix.h @@ +155,5 @@ > void _dbus_fd_set_close_on_exec (int fd); > > +typedef enum > +{ > + DBUS_OPEN_STDIN_NULL = (1 << 0), Replace OPEN with FORCE.
Comment on attachment 125164 [details] [review] dbus-daemon, dbus-launch: cope with callers having closed standard fds Review of attachment 125164 [details] [review]: ----------------------------------------------------------------- Ship it. Reviewed-by: Thiago Macieira <thiago@kde.org>
Comment on attachment 125165 [details] [review] _dbus_become_daemon: use _dbus_open_standard_fds, and report errors Review of attachment 125165 [details] [review]: ----------------------------------------------------------------- Ship it. Reviewed-by: Thiago Macieira <thiago@kde.org>
Comment on attachment 125166 [details] [review] dbus-launch: use _dbus_open_standard_fds() in the babysitter Review of attachment 125166 [details] [review]: ----------------------------------------------------------------- I'd squash onto the previous commit, but this is good too. Ship it. Reviewed-by: Thiago Macieira <thiago@kde.org>
Comment on attachment 125167 [details] [review] dbus-launch: use _dbus_open_standard_fds when closing stderr Review of attachment 125167 [details] [review]: ----------------------------------------------------------------- Ship it. Reviewed-by: Thiago Macieira <thiago@kde.org>
Comment on attachment 125168 [details] [review] _read_subprocess_line_argv: use _dbus_open_standard_fds Review of attachment 125168 [details] [review]: ----------------------------------------------------------------- Looks good, suggestion is just suggestion. Otherwise, Reviewed-by: Thiago Macieira <thiago@kde.org> ::: dbus/dbus-sysdeps-unix.c @@ +3609,5 @@ > + > + /* Try to write details into the pipe, but don't bother > + * trying too hard (no retry loop). */ > + > + if (write (errors_pipe[WRITE_END], error_str, strlen (error_str)) < 0 || I'd use a writev here to ensure that everything is written in one go. Just in case the reading end is too dumb to read until EOF.
(In reply to Thiago Macieira from comment #12) > Which order do the patches go? The order I attached them in. (In reply to Thiago Macieira from comment #13) > Not familiar with the test infra. It's Automake's standard support for TAP (<http://testanything.org/>). Many of the tests use GLib's test library, but these ones were much easier to implement as shell scripts, and they're Unix-specific anyway so that's no loss. The short version is that nonzero exit status or a line on stdout starting "not ok" is a failure, a line on stdout starting "ok" is a success, and the "plan" (echo 1..3) indicates how many successes we expect to have (so the test harness can report a failure if it sees fewer). (In reply to Thiago Macieira from comment #13) > > +# Copyright © 2016 Collabora Ltd. > > Splinter can't display UTF-8? WTF? We're in 2016.... Yeah, seriously. I assure you it's correct UTF-8 in the version on my laptop. (In reply to Thiago Macieira from comment #14) > I'd name it "ensure" instead of "open". If you passed 0 for flags, it > wouldn't open anything. Good idea, "ensure" describes it better. > This can only fail if another thread closed an fd that we just opened. > That's brokenness on the other thread, so ok here. Right, and the doc-comment specifically says you can't call this in a multi-threaded state. > Replace OPEN with FORCE. Good thinking.
(In reply to Thiago Macieira from comment #17) > I'd squash onto the previous commit Since this is non-essential, I wasn't going to include this one in 1.10. (In reply to Thiago Macieira from comment #19) > I'd use a writev here to ensure that everything is written in one go. Just > in case the reading end is too dumb to read until EOF. We control the reading end, and it calls _dbus_read() in a loop, so I think this is fine.
(In reply to Simon McVittie from comment #21) > (In reply to Thiago Macieira from comment #19) > > I'd use a writev here to ensure that everything is written in one go. Just > > in case the reading end is too dumb to read until EOF. > > We control the reading end, and it calls _dbus_read() in a loop, so I think > this is fine. Also, I don't know whether writev() is 100% portable (elsewhere in libdbus we assume it isn't), and this sort of rarely-invoked error-handling code path seems a bad place to add the complexity of a fallback.
Created attachment 125207 [details] [review] test: add a regression test for eval "$(dbus-launch --sh-syntax)" Signed-off-by: Simon McVittie <smcv@debian.org>
Created attachment 125208 [details] [review] Add a simple test for dbus-daemon --fork Signed-off-by: Simon McVittie <smcv@debian.org>
Created attachment 125210 [details] [review] _dbus_ensure_standard_fds: new function to ensure std* fds are open This function opens stdin, stdout, stderr pointing to /dev/null if they aren't already open. Optionally, it can also replace whatever is available on those fds with /dev/null. To allow for use in contexts where only async-signal-safe functions should be used, such as between fork() and a following exec(), this function does not use conventional libdbus error handling (which would require malloc). Instead, it sets errno and returns an explanatory string. Signed-off-by: Simon McVittie <smcv@debian.org> --- v2: rename from "open" to "ensure" and from DBUS_OPEN_STDFOO_NULL to DBUS_FORCE_STDFOO_NULL.
Created attachment 125212 [details] [review] dbus-daemon, dbus-launch: cope with callers having closed standard fds In Debian bug <https://bugs.debian.org/829348>, lightdm appears to have been starting dbus-launch with at least one of the three standard fds 0, 1, 2 (stdin, stdout, stderr) closed. This resulted in the dbus-daemon's epoll_create1() returning a fd less than 3. Later, _dbus_become_daemon() replaces fds 0-2 with /dev/null. As a result, a subsequent call to _dbus_loop_add_watch() for the reload pipe resulted in calling epoll_ctl on the non-epoll fd pointing to /dev/null, which fails with EINVAL, resulting in the dbus-daemon exiting unsuccessfully. Unix programs are not normally expected to behave correctly when launched with the standard fds not already open; but at the same time, X11 autolaunching means that dbus-launch (and hence the dbus-daemon) can get started from an arbitrarily precarious situation. Signed-off-by: Simon McVittie <smcv@debian.org> --- Rebase on new API
Created attachment 125213 [details] [review] _dbus_become_daemon: use _dbus_ensure_standard_fds, and report errors Signed-off-by: Simon McVittie <smcv@debian.org> --- Rebase on new API
Created attachment 125215 [details] [review] dbus-launch: use _dbus_ensure_standard_fds() in the babysitter Signed-off-by: Simon McVittie <smcv@debian.org> --- Rebase on new API
Created attachment 125217 [details] [review] dbus-launch: use _dbus_ensure_standard_fds when closing stderr Signed-off-by: Simon McVittie <smcv@debian.org> --- Rebase on new API
Created attachment 125218 [details] [review] _read_subprocess_line_argv: use _dbus_ensure_standard_fds This also gives us an opportunity to improve the error reporting. Signed-off-by: Simon McVittie <smcv@debian.org> --- Rebase on new API
Created attachment 125219 [details] [review] test: expand dbus-launch-eval test to cover stdin being closed Signed-off-by: Simon McVittie <smcv@debian.org>
Created attachment 125220 [details] [review] test-dbus-daemon-fork: exercise closed stdin, stdout, stderr Signed-off-by: Simon McVittie <smcv@debian.org>
Created attachment 125221 [details] [review] Add a regression test for dbus-launch in X11 Signed-off-by: Simon McVittie <smcv@debian.org> --- Not 100% related, but it's about time we actually had some coverage for this, given that distributions rely on it working.
Comment on attachment 125210 [details] [review] _dbus_ensure_standard_fds: new function to ensure std* fds are open Review of attachment 125210 [details] [review]: ----------------------------------------------------------------- Ship it.
Comment on attachment 125212 [details] [review] dbus-daemon, dbus-launch: cope with callers having closed standard fds Review of attachment 125212 [details] [review]: ----------------------------------------------------------------- Ship it.
Comment on attachment 125213 [details] [review] _dbus_become_daemon: use _dbus_ensure_standard_fds, and report errors Review of attachment 125213 [details] [review]: ----------------------------------------------------------------- Ship it.
Comment on attachment 125215 [details] [review] dbus-launch: use _dbus_ensure_standard_fds() in the babysitter Review of attachment 125215 [details] [review]: ----------------------------------------------------------------- Ship it.
Comment on attachment 125217 [details] [review] dbus-launch: use _dbus_ensure_standard_fds when closing stderr Review of attachment 125217 [details] [review]: ----------------------------------------------------------------- Ship it
Comment on attachment 125218 [details] [review] _read_subprocess_line_argv: use _dbus_ensure_standard_fds Review of attachment 125218 [details] [review]: ----------------------------------------------------------------- Understood. Ship it.
All the C changes look fine. You can add my name as the reviewer. I'm afraid you'll have to do without me for the shell parts.
(In reply to Thiago Macieira from comment #40) > All the C changes look fine. You can add my name as the reviewer. Thanks. Push access to fd.o dbus.git is currently broken (Bug #97014) so I'm pushing to the mirror on GitHub for now, and I'll resync back to fd.o when Bug #97014 is fixed. > I'm afraid you'll have to do without me for the shell parts. Colin, Philip, I hear you like regression tests? Or if nobody reviews them for a couple of weeks I'll probably just push them to master unreviewed: they pass on my laptop and on Travis-CI, and imperfect test coverage seems better than no test coverage.
Comment on attachment 125207 [details] [review] test: add a regression test for eval "$(dbus-launch --sh-syntax)" Review of attachment 125207 [details] [review]: ----------------------------------------------------------------- One minor comment. ::: test/test-dbus-launch-eval.sh @@ +48,5 @@ > + > +eval "$(${DBUS_TEST_DBUS_LAUNCH} --sh-syntax "$config")" > + > +test -n "$DBUS_SESSION_BUS_ADDRESS" > +env | grep '^DBUS_SESSION_BUS_ADDRESS=' The test below for the _PID case looks stronger (without -o pipefail).
Comment on attachment 125208 [details] [review] Add a simple test for dbus-daemon --fork Review of attachment 125208 [details] [review]: ----------------------------------------------------------------- OK.
(In reply to Colin Walters from comment #42) > The test below for the _PID case looks stronger (without -o pipefail). It's a different test. For DBUS_SESSION_BUS_ADDRESS, I'm asserting that it is non-empty and exported. For DBUS_SESSION_BUS_PID, I'm asserting that it is non-empty and is *not* exported. Real output looks something like this: DBUS_SESSION_BUS_ADDRESS='unix:abstract=/tmp/dbus-JwZPYB6mMB,guid=31a67e155c4bba3de2a76a965795fb7f'; export DBUS_SESSION_BUS_ADDRESS; DBUS_SESSION_BUS_PID=11510; DBUS_SESSION_BUS_WINDOWID=2097153;
Comment on attachment 125210 [details] [review] _dbus_ensure_standard_fds: new function to ensure std* fds are open Review of attachment 125210 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-unix.c @@ +169,5 @@ > + { > + /* Because we rely on being single-threaded, and we want the > + * standard fds to not be close-on-exec, we don't set it > + * close-on-exec. */ > + if (devnull < i) I find myself being misled by this conditional - given it will *always* be hit, it seems the code would be clearer if we simply did: ``` devnull = open ("/dev/null", O_RDWR); if (devnull < 0) ... ``` before the loop. (Though this gets more complex with my next comment) Then we can work out more clearly whether we need to dup2(). @@ +170,5 @@ > + /* Because we rely on being single-threaded, and we want the > + * standard fds to not be close-on-exec, we don't set it > + * close-on-exec. */ > + if (devnull < i) > + devnull = open ("/dev/null", O_RDWR); While I can't imagine why this would be a problem, it seems a bit weird to potentially have stdin be writable and stdout be readable. Similar code in glib explicitly does O_RDONLY and O_WRONLY for example. (combining the above, maybe open O_RDONLY, see if it's > 1 && < 3, if so reopen O_RDWR ?)
(In reply to Colin Walters from comment #45) > I find myself being misled by this conditional - given it will *always* be > hit, > it seems the code would be clearer if we simply did: > > ``` > devnull = open ("/dev/null", O_RDWR); > if (devnull < 0) > ... > ``` > > before the loop. (Though this gets more complex with my next > comment) Yes, it's possible to push the first open() out of the loop, but we have to be prepared to reopen /dev/null inside the loop *anyway*, because it's how we test what the next previously-closed standard fd is - unless we are willing to use fcntl() on possibly-closed fds and test whether we get EBADFD as our way to do that. With that in mind, keeping every open() in the loop (and making sure the first one is always done) is a net simplification. I deliberately avoided doing the fcntl/EBADFD thing because tools like valgrind frequently flag any operation on a closed fd as an error, and it would make readers worry about time-of-check/time-of-use (although this code is specified to be run single-threaded so in practice there's no TOC/TOU here). > While I can't imagine why this would be a problem, it seems a bit weird to > potentially have stdin be writable and stdout be readable. Similar code in > glib explicitly does O_RDONLY and O_WRONLY for example. Similar code in libdbus consistently used O_RDWR (look at the patches where I replaced existing code with calls to this function). My first version of this code did carefully open stdin read-only and stdout, stderr write-only, but it added complexity (the initial function was about 3 times this length and relied on the three standard fds being in this specific order), and I wasn't really convinced there was any practical benefit. The versions that I attached here were a simpler rewrite that I hoped would be kinder to reviewers by being more obviously correct, instead of "you can satisfy yourself that it's correct if you stare at it for a while" :-)
(In reply to Simon McVittie from comment #46) > My first version of this code did carefully open stdin read-only and stdout, > stderr write-only, but it added complexity (the initial function was about 3 > times this length and relied on the three standard fds being in this > specific order), and I wasn't really convinced there was any practical > benefit. The versions that I attached here were a simpler rewrite that I > hoped would be kinder to reviewers by being more obviously correct, instead > of "you can satisfy yourself that it's correct if you stare at it for a > while" :-) Ok, makes sense. Though I still found myself staring at it =)
I've merged the tests (mostly) unreviewed, because it's been a few weeks, and test coverage for this stuff is better than no test coverage. (Most of this is functionality that has existed for about a decade and should have been tested by now.) Please revert, reopen and/or propose changes if there is something you object to. Fixed in git for 1.10.10, 1.11.4. On the 1.10 branch, the new tests needed DBUS_USE_TEST_BINARY set, because that branch doesn't have commit c7f3df0 from Bug #92899. Other than that, they work as-is. (In reply to Simon McVittie from comment #33) > Add a regression test for dbus-launch in X11 I modified this test from the previously-attached version to start an X server that will run for up to 5 minutes, instead of up to 1 hour. On master, I also added a fairly obvious extension to this test to cover the new --exit-with-x11 option.
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.