Bug 97008 - dbus-daemon --fork fails to start if stdin is closed
Summary: dbus-daemon --fork fails to start if stdin is closed
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.10
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review? for tests
Keywords: patch
Depends on: 97014
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-20 17:06 UTC by Simon McVittie
Modified: 2016-08-12 17:48 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
test: add a regression test for eval "$(dbus-launch --sh-syntax)" (5.75 KB, patch)
2016-07-20 17:07 UTC, Simon McVittie
Details | Splinter Review
Add a simple test for dbus-daemon --fork (3.43 KB, patch)
2016-07-20 17:08 UTC, Simon McVittie
Details | Splinter Review
_dbus_open_standard_fds: new function to ensure std* fds are open (3.95 KB, patch)
2016-07-20 17:09 UTC, Simon McVittie
Details | Splinter Review
dbus-daemon, dbus-launch: cope with callers having closed standard fds (3.67 KB, patch)
2016-07-20 17:10 UTC, Simon McVittie
Details | Splinter Review
_dbus_become_daemon: use _dbus_open_standard_fds, and report errors (1.94 KB, patch)
2016-07-20 17:10 UTC, Simon McVittie
Details | Splinter Review
dbus-launch: use _dbus_open_standard_fds() in the babysitter (1.91 KB, patch)
2016-07-20 17:11 UTC, Simon McVittie
Details | Splinter Review
dbus-launch: use _dbus_open_standard_fds when closing stderr (1.28 KB, patch)
2016-07-20 17:11 UTC, Simon McVittie
Details | Splinter Review
_read_subprocess_line_argv: use _dbus_open_standard_fds (1.98 KB, patch)
2016-07-20 17:12 UTC, Simon McVittie
Details | Splinter Review
test: expand dbus-launch-eval test to cover stdin being closed (2.02 KB, patch)
2016-07-20 17:12 UTC, Simon McVittie
Details | Splinter Review
test: expand dbus-launch-eval test to cover stdin being closed (2.08 KB, patch)
2016-07-20 17:13 UTC, Simon McVittie
Details | Splinter Review
test: add a regression test for eval "$(dbus-launch --sh-syntax)" (5.74 KB, patch)
2016-07-21 10:21 UTC, Simon McVittie
Details | Splinter Review
Add a simple test for dbus-daemon --fork (3.42 KB, patch)
2016-07-21 10:21 UTC, Simon McVittie
Details | Splinter Review
_dbus_ensure_standard_fds: new function to ensure std* fds are open (4.29 KB, patch)
2016-07-21 10:21 UTC, Simon McVittie
Details | Splinter Review
dbus-daemon, dbus-launch: cope with callers having closed standard fds (3.67 KB, patch)
2016-07-21 10:22 UTC, Simon McVittie
Details | Splinter Review
_dbus_become_daemon: use _dbus_ensure_standard_fds, and report errors (1.94 KB, patch)
2016-07-21 10:22 UTC, Simon McVittie
Details | Splinter Review
dbus-launch: use _dbus_ensure_standard_fds() in the babysitter (1.91 KB, patch)
2016-07-21 10:23 UTC, Simon McVittie
Details | Splinter Review
dbus-launch: use _dbus_ensure_standard_fds when closing stderr (1.28 KB, patch)
2016-07-21 10:23 UTC, Simon McVittie
Details | Splinter Review
_read_subprocess_line_argv: use _dbus_ensure_standard_fds (1.98 KB, patch)
2016-07-21 10:24 UTC, Simon McVittie
Details | Splinter Review
test: expand dbus-launch-eval test to cover stdin being closed (2.01 KB, patch)
2016-07-21 10:25 UTC, Simon McVittie
Details | Splinter Review
test-dbus-daemon-fork: exercise closed stdin, stdout, stderr (2.08 KB, patch)
2016-07-21 10:25 UTC, Simon McVittie
Details | Splinter Review
Add a regression test for dbus-launch in X11 (10.45 KB, patch)
2016-07-21 10:26 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2016-07-20 17:06:25 UTC
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.
Comment 1 Simon McVittie 2016-07-20 17:07:56 UTC
Created attachment 125161 [details] [review]
test: add a regression test for eval "$(dbus-launch  --sh-syntax)"
Comment 2 Simon McVittie 2016-07-20 17:08:26 UTC
Created attachment 125162 [details] [review]
Add a simple test for dbus-daemon --fork
Comment 3 Simon McVittie 2016-07-20 17:09:16 UTC
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.
Comment 4 Simon McVittie 2016-07-20 17:10:18 UTC
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.
Comment 5 Simon McVittie 2016-07-20 17:10:59 UTC
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.
Comment 6 Simon McVittie 2016-07-20 17:11:20 UTC
Created attachment 125166 [details] [review]
dbus-launch: use _dbus_open_standard_fds() in the  babysitter

---

1.11 only.
Comment 7 Simon McVittie 2016-07-20 17:11:41 UTC
Created attachment 125167 [details] [review]
dbus-launch: use _dbus_open_standard_fds when closing  stderr

---

1.11 only.
Comment 8 Simon McVittie 2016-07-20 17:12:10 UTC
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.
Comment 9 Simon McVittie 2016-07-20 17:12:53 UTC
Created attachment 125169 [details] [review]
test: expand dbus-launch-eval test to cover stdin being  closed
Comment 10 Simon McVittie 2016-07-20 17:13:07 UTC
Created attachment 125170 [details] [review]
test: expand dbus-launch-eval test to cover stdin being  closed
Comment 11 Simon McVittie 2016-07-20 17:13:52 UTC
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.
Comment 12 Thiago Macieira 2016-07-21 00:28:47 UTC
Which order do the patches go?
Comment 13 Thiago Macieira 2016-07-21 00:31:43 UTC
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 14 Thiago Macieira 2016-07-21 00:39:17 UTC
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 15 Thiago Macieira 2016-07-21 00:41:21 UTC
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 16 Thiago Macieira 2016-07-21 00:43:09 UTC
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 17 Thiago Macieira 2016-07-21 00:44:08 UTC
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 18 Thiago Macieira 2016-07-21 00:45:02 UTC
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 19 Thiago Macieira 2016-07-21 00:47:38 UTC
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.
Comment 20 Simon McVittie 2016-07-21 07:11:16 UTC
(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.
Comment 21 Simon McVittie 2016-07-21 07:17:16 UTC
(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.
Comment 22 Simon McVittie 2016-07-21 09:08:02 UTC
(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.
Comment 23 Simon McVittie 2016-07-21 10:21:10 UTC
Created attachment 125207 [details] [review]
test: add a regression test for eval "$(dbus-launch --sh-syntax)"

Signed-off-by: Simon McVittie <smcv@debian.org>
Comment 24 Simon McVittie 2016-07-21 10:21:17 UTC
Created attachment 125208 [details] [review]
Add a simple test for dbus-daemon --fork

Signed-off-by: Simon McVittie <smcv@debian.org>
Comment 25 Simon McVittie 2016-07-21 10:21:57 UTC
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.
Comment 26 Simon McVittie 2016-07-21 10:22:21 UTC
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
Comment 27 Simon McVittie 2016-07-21 10:22:34 UTC
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
Comment 28 Simon McVittie 2016-07-21 10:23:34 UTC
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
Comment 29 Simon McVittie 2016-07-21 10:23:47 UTC
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
Comment 30 Simon McVittie 2016-07-21 10:24:12 UTC
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
Comment 31 Simon McVittie 2016-07-21 10:25:28 UTC
Created attachment 125219 [details] [review]
test: expand dbus-launch-eval test to cover stdin being closed

Signed-off-by: Simon McVittie <smcv@debian.org>
Comment 32 Simon McVittie 2016-07-21 10:25:38 UTC
Created attachment 125220 [details] [review]
test-dbus-daemon-fork: exercise closed stdin, stdout, stderr

Signed-off-by: Simon McVittie <smcv@debian.org>
Comment 33 Simon McVittie 2016-07-21 10:26:14 UTC
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 34 Thiago Macieira 2016-07-22 21:07:03 UTC
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 35 Thiago Macieira 2016-07-22 21:07:44 UTC
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 36 Thiago Macieira 2016-07-22 21:08:20 UTC
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 37 Thiago Macieira 2016-07-22 21:08:50 UTC
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 38 Thiago Macieira 2016-07-22 21:09:31 UTC
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 39 Thiago Macieira 2016-07-22 21:10:39 UTC
Comment on attachment 125218 [details] [review]
_read_subprocess_line_argv: use _dbus_ensure_standard_fds

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

Understood. Ship it.
Comment 40 Thiago Macieira 2016-07-22 21:11:19 UTC
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.
Comment 41 Simon McVittie 2016-07-25 10:37:56 UTC
(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 42 Colin Walters 2016-07-25 11:08:13 UTC
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 43 Colin Walters 2016-07-25 11:09:48 UTC
Comment on attachment 125208 [details] [review]
Add a simple test for dbus-daemon --fork

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

OK.
Comment 44 Simon McVittie 2016-07-25 11:44:24 UTC
(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 45 Colin Walters 2016-07-25 12:57:30 UTC
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 ?)
Comment 46 Simon McVittie 2016-07-25 18:15:15 UTC
(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" :-)
Comment 47 Colin Walters 2016-07-25 18:51:40 UTC
(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 =)
Comment 48 Simon McVittie 2016-08-12 17:48:47 UTC
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.