Created attachment 93633 [details] [review] Proposed patch If an X error occurs before the main dbus-launch process transmits the daemon's PID to the babysitter, x_io_error_handler will try to kill the bus with bus_pid_to_kill still set to -1. This will result in killing every process that dbus-launch can get its hands on. I can easily reproduce this by running xvfb-run sh -c 'dbus-launch --autolaunch=11111111111111111111111111111111 &' a bunch of times. The attached patch fixes this by making sure bus_pid_to_kill doesn't try to kill -1. It also fixes a related problem: if the X error happens after the fork but before the babysitter gets the bus PID, the bus won't be killed, because bus_pid_to_kill isn't initialized yet.
Comment on attachment 93633 [details] [review] Proposed patch Review of attachment 93633 [details] [review]: ----------------------------------------------------------------- This seems a good change, yes. A couple of coding style points, and I'd prefer the check for not-really-a-pid things to be a bit more comprehensive: ::: tools/dbus-launch.c @@ +406,4 @@ > static void > kill_bus(void) > { > + if (bus_pid_to_kill == -1) return; D-Bus coding style is approximately GNU style, the same as in GNOME: if (bus_pid_to_kill == -1) return; Also, I'd prefer it if this checked for "is a real pid" vs. "is some sort of pseudo-pid": if (bus_pid_to_kill <= 0) return; (The IDs of actual processes are strictly positive: pid < -1, pid == -1 and pid == 0 are all special values, see kill(2).) @@ +1275,5 @@ > } > > + /* Have to initialize bus_pid_to_kill ASAP, so that the > + X error callback can kill it if an error happens. */ > + bus_pid_to_kill = bus_pid = val; We don't usually use chained assignments, so I'd prefer it like this: bus_pid = val; /* Have to initialize [...] error happens. */ bus_pid_to_kill = val;
(In reply to comment #0) > If an X error occurs before the main dbus-launch process transmits the > daemon's PID to the babysitter Which X error can occur under normal circumstances? > This will result in killing every > process that dbus-launch can get its hands on. That's obviously bad, but dbus-launch should do *something* in response to an X error: if it hasn't forked the child that will execute dbus-daemon yet, it should issue a warning to stderr (or possibly syslog) and exit with nonzero status. Does it? I can't help thinking that dbus-launch ought to be simpler - I'm not sure why the babysitter is the "bus runner"'s sibling, rather than its parent...
At a high level, what we want is: when something goes fatally wrong (X error, D-Bus error, ...), all the dbus-launch processes that have been started exit, and so does the dbus-daemon. (In reply to comment #0) > I can easily reproduce this by running xvfb-run sh -c 'dbus-launch > --autolaunch=11111111111111111111111111111111 &' a bunch of times. It looks as though what you're doing here is manufacturing a race condition: * xvfb-run starts an X server * "sh -c" runs dbus-launch in the background * "sh -c" has nothing more to do, and exits * RACE: * dbus-launch connects to the X server * xvfb-run detects that "sh -c" has exited, and tells the X server to exit If dbus-launch wins the race then it connects to the X server before the X server exits, sets up its dbus-daemon, receives an X error, kills its dbus-daemon and exits. All good. If xvfb-run wins the race then dbus-launch gets an X error before it has forked, and does the incorrect kill(-1). It's bad that dbus-launch kills unrelated processes, but I don't see how this would happen "in real life"? With your patch, are the child processes (bus-runner/dbus-daemon, babysitter, "intermediate parent") guaranteed to exit or be killed?
Regarding the coding style issues - sure, I'll upload a reformatted version of the patch. I don't think bus_pid_to_kill can ever contain any special value other than -1 (unless dbus-daemon decides to sabotage it), but I suppose for extra safety it may as well be a more broad check. (In reply to comment #2) > (In reply to comment #0) > > If an X error occurs before the main dbus-launch process transmits the > > daemon's PID to the babysitter > > Which X error can occur under normal circumstances? An error is abnormal pretty much by definition; I don't know what you're asking here. > That's obviously bad, but dbus-launch should do *something* in response to > an X error: if it hasn't forked the child that will execute dbus-daemon yet, > it should issue a warning to stderr (or possibly syslog) and exit with > nonzero status. Does it? No, not currently. I suppose x_io_error_handler can easily be amended to do that; although it can't distinguish between being in the main process and being in the babysitter, so the babysitter will have the same response to an X error. > I can't help thinking that dbus-launch ought to be simpler - I'm not sure > why the babysitter is the "bus runner"'s sibling, rather than its parent... So that it doesn't become a zombie child of the main process. (In reply to comment #3) > It looks as though what you're doing here is manufacturing a race condition: Exactly. > If xvfb-run wins the race then dbus-launch gets an X error before it has > forked, and does the incorrect kill(-1). This can also happen if dbus-launch gets an X error after it has forked but before it has sent the bus's PID to the babysitter. > It's bad that dbus-launch kills unrelated processes, but I don't see how > this would happen "in real life"? I did actually encounter this bug in real life - I was running a graphical program with xvfb-run that indirectly ran dbus-launch (not sure how exactly that worked, because I stopped debugging once I found the culprit - it had to do with Ubuntu's overlay scrollbars, GSettings and dconf), and occasionally it would do kill(-1). > With your patch, are the child processes (bus-runner/dbus-daemon, > babysitter, "intermediate parent") guaranteed to exit or be killed? I think so. If the X error occurs before the fork, there won't be any child processes to begin with. If it occurs after the fork, then the bus runner and the intermediate parent will exit normally, the main process will kill the bus daemon and exit, and the babysitter will exit because bus_pid_to_babysitter_pipe will be closed.
Created attachment 93802 [details] [review] Proposed patch v2
Ping? Anything else you want me to do?
> (In reply to comment #2) > > Which X error can occur under normal circumstances? > > An error is abnormal pretty much by definition; I don't know what you're > asking here. I suppose what I was really asking is: what's (an example of) the real-world situation in which an X error can occur at a time that triggers this bug? Or is it purely theoretical? - and you answered that already. > > That's obviously bad, but dbus-launch should do *something* in response to > > an X error: if it hasn't forked the child that will execute dbus-daemon yet, > > it should issue a warning to stderr (or possibly syslog) and exit with > > nonzero status. Does it? > > No, not currently. I suppose x_io_error_handler can easily be amended to do > that; although it can't distinguish between being in the main process and > being in the babysitter, so the babysitter will have the same response to an > X error. I would like to be sure that our response to an X error is never "oh well, carry on regardless"; dbus-launch must always respond to an X error by either killing the dbus-daemon, or making sure that it will not start the dbus-daemon. If your current proposed patch is intended to do that, I'll try to find time to trace through the various code paths and make sure it does. > > I can't help thinking that dbus-launch ought to be simpler - I'm not sure > > why the babysitter is the "bus runner"'s sibling, rather than its parent... > > So that it doesn't become a zombie child of the main process. I can see that that's why it doesn't go like this: shell [*] \- main() --exec--> myapp[*] \- babysitter[*] \- bus-runner --exec--> dbus-daemon --fork \- final dbus-daemon[*] [*]: long-lived process because if the babysitter gets an X error and exits, myapp might never reap it and so it'll become a zombie. I don't see why not this, though: shell [*] \- main() --exec--> myapp[*] \- "intermediate parent" \- babysitter[*] \- bus-runner --exec--> dbus-daemon --fork \- final dbus-daemon[*] or indeed this: shell [*] \- main() --exec--> myapp[*] \- "intermediate parent" \- babysitter[*] \- bus-runner --exec--> dbus-daemon --nofork If one of those makes it easier to get this right, I'd consider that patch.
> If your current proposed patch is intended to do that Yep, that is the purpose of the 2nd and 3rd hunks. In fact, I did test that with just the 1st hunk, my original test case sometimes leaves dbus-daemon processes running, and with all 3 it doesn't. I did find another suspicious spot just now, though: in kill_bus_when_session_ends, in the branch that prints "No terminal on standard input and no X display" the exit(1) should probably be a kill_bus_and_exit(1). Would you agree? > I don't see why not this, though: I see what you mean, and I have no idea. :-) I don't think this has any bearing on the applicability of this patch, anyway.
(In reply to comment #8) > I did find another suspicious spot just now, though: in > kill_bus_when_session_ends, in the branch that prints "No terminal on > standard input and no X display" the exit(1) should probably be a > kill_bus_and_exit(1). Would you agree? Yes. Please do/test that. > [ensuring death of the dbus-daemon] is the purpose of the 2nd and 3rd hunks. > In fact, I did test that > with just the 1st hunk, my original test case sometimes leaves dbus-daemon > processes running, and with all 3 it doesn't. Your patch does appear to be correct, but the fact that it is correct is far too subtle (indeed, everything about dbus-launch is far too subtle). The timeline of the parent dbus-launch process goes something like this: * do initial X calls [1] * do some other stuff * fork (child process starts doing some other stuff) * return "intermediate parent" pid from fork() * obtain bus daemon pid from bus_pid_to_launcher_pipe [2] * do things that might include X11 calls or killing the dbus-daemon Meanwhile, the babysitter goes like this: * return 0 from fork() [3] * obtain bus daemon pid from parent process via bus_pid_to_babysitter_pipe [4] * do things that might include X11 calls or killing the bus daemon AFAICS, the 1st hunk of your patch relies for its correctness on the fact that control does not pass into libX11 in any way between [1] and [2], or between [3] and [4], so there's no way we can receive an error indication and try to kill the dbus-daemon during that interval. If we did, we would previously have done a kill(-1) and blown up the user session, which is at least visible. Now, we do nothing. The correct thing to do about an X error is: * before [1] or [3]: don't kill anything. Current implementation wrongly kills -1, your implementation is right. * after [2] or [4]: kill the dbus-daemon. Current implementation wrongly kills -1 in rare cases, your implementation is right. * between [1] and [2], or [3] and [4]: there is no correct thing we can do so we'd better be really sure it can't happen. Current implementation would kill -1, your implementation would not kill anything, both are wrong. This is pretty annoying. The only resolutions I can think of are: * add a comment briefly explaining the situation (with a reference to this bug) and make really sure we never add any libX11 calls in those critical sections in future * track whether we are in one of those critical sections (maybe via bus_pid_to_kill being 0, -1 or positive to mean "bus not started", "bus potentially started but pid unknown" or "here is its pid", or via a separate tristate variable), and if we are, defer exiting until we have tried to discover the right pid to kill Any thoughts/ideas from the other maintainers? This reinforces the opinion I have had for a while that dbus-launch should be specific to dbus-on-X11 and deprecated in favour of systemd user sessions and/or dbus-run-session, and that in the glorious and semi-hypothetical Wayland (and/or Mir and/or SurfaceFlinger) future, dbus-launch should just not exist.
Comment on attachment 93802 [details] [review] Proposed patch v2 Review of attachment 93802 [details] [review]: ----------------------------------------------------------------- Looks good.
The contents of the patch are ok by itself, since we definitely do not want to ever kill -1. I really don't remember what the convoluted babysitter code is doing. I also don't believe I fully understood it when I wrote the X launcher back in 2006. This patch is better than nothing. More comments explaining what to do later or not do would be welcome.
Fixed in git for 1.8.2, 1.9.0. Leaving this bug open with a new title.
(In reply to comment #9) > (In reply to comment #8) > > I did find another suspicious spot just now, though: in > > kill_bus_when_session_ends, in the branch that prints "No terminal on > > standard input and no X display" the exit(1) should probably be a > > kill_bus_and_exit(1). Would you agree? > > Yes. Please do/test that. OK, I'll attach the patch in a moment. I tested that running "dbus-launch --exit-with-session </dev/null" without the change leaves a dbus-daemon process running; with the change, that process is killed.
Created attachment 98341 [details] [review] Kill bus if can't attach to session
Comment on attachment 98341 [details] [review] Kill bus if can't attach to session Review of attachment 98341 [details] [review]: ----------------------------------------------------------------- Looks good, I'll apply it for 1.8.4
(In reply to comment #15) > Looks good, I'll apply it for 1.8.4 1.8.4 was a security release with only one change. I'll apply it for 1.8.6 instead.
Comment on attachment 93802 [details] [review] Proposed patch v2 was applied, marking obsolete
Comment on attachment 98341 [details] [review] Kill bus if can't attach to session Fixed in git for 1.8.6, 1.9.0. As a note for the future, it's helpful if you can provide patches in "git format-patch" format so that we can attribute them correctly.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/96.
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.