On Unix, dbus-spawn works like this: main process | fork() A \- babysitter process | fork () B \- grandchild process --> exec --> spawned process and the child_setup() callback is called in the grandchild just before the exec. This is the ideal time to modify global process state: setrlimit(), chdir(), all that sort of thing. On Windows, instead it works like this, because Windows does not have fork(), exec(), or the same rigid structure of parent and child processes as Unix: ---------------------------------------------- main process's main thread | CreateThread() main process \- babysitter thread ---| CreateProcessA() ------------------------ \- spawned main thread spawned process and the child setup() callback is called in the babysitter thread, part of the main process! This makes most Unix uses of a child_setup() function actively harmful: for instance, because the babysitter thread is in the same process as the main thread, SetCurrentDirectory() would set the entire dbus-daemon's current directory. I think dbus-spawn-win.c should just accept and ignore the child_setup() function: not save it in a struct, and not call it.
Created attachment 120088 [details] [review] Do not run babysitter child_setup function on Windows because it runs in the parent application.
Comment on attachment 120088 [details] [review] Do not run babysitter child_setup function on Windows because it runs in the parent application. Review of attachment 120088 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-spawn-win.c @@ +591,4 @@ > if (sitter->child_setup) > { > PING(); > +#ifdef DBUS_WIN We're compiling dbus-spawn-win.c. Why would DBUS_WIN not be defined? :-)
(In reply to Simon McVittie from comment #2) > Comment on attachment 120088 [details] [review] [review] > Do not run babysitter child_setup function on Windows because it runs in the > parent application. > > Review of attachment 120088 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-spawn-win.c > @@ +591,4 @@ > > if (sitter->child_setup) > > { > > PING(); > > +#ifdef DBUS_WIN > > We're compiling dbus-spawn-win.c. Why would DBUS_WIN not be defined? :-) yes DBUS_WIN is completly obsolate. I better should have used DBUS_WIN_FIXME because child setup is solvable in the case the child links to libdbus. In theory child_setup could be called in libdbus-1 startup code (DLLMain), but it need to be informed from the parent, that this case is present and which function to call.
(In reply to Ralf Habacker from comment #3) > I better should have used DBUS_WIN_FIXME because child setup is solvable in > the case the child links to libdbus. In general it won't - for instance, activated D-Bus services will often use a different implementation like GDBus or dbus-sharp. A feature that can't be relied on is worse than no feature at all, in some ways. > In theory child_setup could be called > in libdbus-1 startup code (DLLMain), but it need to be informed from the > parent, that this case is present and which function to call. I don't think this is worth putting any effort into. The child_setup() callback only makes sense in the Unix process-execution model; that's fine, we should just document that and move on. See g_spawn_async_with_pipes() in GLib, which is basically the same thing (the early dbus authors were mostly GLib developers and followed similar conventions). It documents child_setup() as accepted but entirely ignored on Windows. In all of dbus, we only have two calls to a non-NULL child_setup function. One is in test code (spawn-test.c), and the other is in bus/activation.c and its contents are entirely Unix-specific (#ifdef DBUS_UNIX).
Created attachment 120137 [details] [review] In _dbus_spawn_async_with_babysitter() remove unsupported implementation of calling child setup function on windows.
Comment on attachment 120137 [details] [review] In _dbus_spawn_async_with_babysitter() remove unsupported implementation of calling child setup function on windows. Review of attachment 120137 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-spawn-win.c @@ +654,5 @@ > *sitter_p = NULL; > > + if (sitter->child_setup) > + { > + _dbus_warn("The child intended to start may not work as expected because child setup function is not supported\n"); No. In practice this is going to crash dbus-daemon every time it tries to activate a service, because warnings are fatal by default, and even on Windows, bus/activation.c defines a child_setup() function - it just doesn't do anything on Windows (its body is #ifdef DBUS_UNIX). Sorry, I did a patch in parallel with yours which documents child_setup as ignored on Windows, but got distracted by other work. I'll attach it now.
Created attachment 120140 [details] [review] Do not attempt to call child_setup on Windows child_setup() is defined to be called after fork() and before exec(), but Windows' process model does not have fork(): the equivalent of those two operations is a single CreateProcess() call. This means that there is no point at which we could call child_setup() and have it affect only the child's process-global state. At the point where it is currently executed, it affects the parent's process-global state instead, which would be actively harmful if we used any child_setup() function that was not a no-op on Windows. The equivalent function in GLib, g_spawn_async_with_pipes(), documents child_setup() as unused on Windows. Do the same here. In practice, our only use of child_setup() outside tests is #ifdef DBUS_UNIX anyway, so this change has no practical effect right now.
Comment on attachment 120140 [details] [review] Do not attempt to call child_setup on Windows Review of attachment 120140 [details] [review]: ----------------------------------------------------------------- looks good, much better than my attempt
The other possibility for this would be to have the signature of the function, or even the whole name of the function, just be different on Windows: /* dbus-spawn.h */ dbus_bool_t _dbus_spawn_async_with_babysitter (..., char **envp, #ifdef DBUS_UNIX DBusSpawnChildSetupFunc child_setup, void *user_data, #endif DBusError *error); /* bus/activation.c */ #ifdef DBUS_UNIX void child_setup (void *user_data) { ... } #endif ... _dbus_spawn_async_with_babysitter (..., envp, #ifdef DBUS_UNIX child_setup, user_data, #endif &error); /* or alternatively... */ #ifdef DBUS_UNIX _dbus_unix_spawn_async_with_babysitter (...); #else _dbus_win_spawn_async_with_babysitter (...); #endif That would mean the compiler would stop us from getting it wrong. But I think that approach is rather horrible, and I'd prefer to just document away the problem (like GLib did).
(In reply to Simon McVittie from comment #9) > That would mean the compiler would stop us from getting it wrong. But I > think that approach is rather horrible, and I'd prefer to just document away > the problem (like GLib did). agreed
(In reply to Ralf Habacker from comment #8) > Comment on attachment 120140 [details] [review] [review] > Do not attempt to call child_setup on Windows > indented for dbus-1.10 ?
(In reply to Ralf Habacker from comment #11) > indented for dbus-1.10 ? No, this does not fix an observable bug, only an area of potential future bugs. Fixed in git for 1.11.0, thanks for reviewing! (In reply to Ralf Habacker from comment #8) > looks good, much better than my attempt To be fair, it's 90% the same as your version - there's a limit to how much creativity can go into deleting some wrong code :-)
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.