Bug 85857 - _dbus_spawn_async_with_babysitter child_setup should not be called on Windows
Summary: _dbus_spawn_async_with_babysitter child_setup should not be called on Windows
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.8
Hardware: Other All
: low minor
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-04 12:42 UTC by Simon McVittie
Modified: 2015-11-27 10:32 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Do not run babysitter child_setup function on Windows because it runs in the parent application. (906 bytes, patch)
2015-11-24 17:03 UTC, Ralf Habacker
Details | Splinter Review
In _dbus_spawn_async_with_babysitter() remove unsupported implementation of calling child setup function on windows. (1.91 KB, patch)
2015-11-26 10:58 UTC, Ralf Habacker
Details | Splinter Review
Do not attempt to call child_setup on Windows (3.69 KB, patch)
2015-11-26 11:47 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 2014-11-04 12:42:01 UTC
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.
Comment 1 Ralf Habacker 2015-11-24 17:03:40 UTC
Created attachment 120088 [details] [review]
Do not run babysitter child_setup function on Windows because it runs in the parent application.
Comment 2 Simon McVittie 2015-11-24 17:47:22 UTC
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? :-)
Comment 3 Ralf Habacker 2015-11-26 07:49:55 UTC
(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.
Comment 4 Simon McVittie 2015-11-26 10:26:28 UTC
(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).
Comment 5 Ralf Habacker 2015-11-26 10:58:19 UTC
Created attachment 120137 [details] [review]
In _dbus_spawn_async_with_babysitter() remove unsupported implementation of calling child setup function on windows.
Comment 6 Simon McVittie 2015-11-26 11:45:33 UTC
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.
Comment 7 Simon McVittie 2015-11-26 11:47:27 UTC
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 8 Ralf Habacker 2015-11-26 11:50:00 UTC
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
Comment 9 Simon McVittie 2015-11-26 11:52:06 UTC
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).
Comment 10 Ralf Habacker 2015-11-26 11:54:26 UTC
(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
Comment 11 Ralf Habacker 2015-11-26 22:05:02 UTC
(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 ?
Comment 12 Simon McVittie 2015-11-27 10:32:26 UTC
(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.