Bug 95160 - Test-spawn unit test crashes on Windows
Summary: Test-spawn unit test crashes on Windows
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.10
Hardware: x86-64 (AMD64) Windows (All)
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-26 21:46 UTC by yfei
Modified: 2016-05-02 11:23 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Fix segfault crash. (1006 bytes, patch)
2016-04-26 21:46 UTC, yfei
Details | Splinter Review
Fix segfault crash when running test-spawn on Windows. (760 bytes, patch)
2016-04-27 15:14 UTC, yfei
Details | Splinter Review
Fix assert in test-spawn caused by missing initialization of DBusError instance on gcc builds. (713 bytes, patch)
2016-04-29 07:51 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description yfei 2016-04-26 21:46:00 UTC
The test-spawn unit test crashes because the call to _dbus_spawn_async_with_babysitter() requires a valid pointer to a DBusBabysitter pointer object, but is passed a NULL pointer instead.
Comment 1 yfei 2016-04-26 21:46:25 UTC
Created attachment 123284 [details] [review]
Fix segfault crash.
Comment 2 Simon McVittie 2016-04-27 10:57:27 UTC
I think I'd prefer to solve this by making the Windows implementation of _dbus_spawn_async_with_babysitter() (in dbus/dbus-spawn-win.c) cope with sitter_p being NULL, like the original Unix implementation (in dbus/dbus-spawn.c) already does.

The Windows implementation doesn't seem to be able to decide whether it supports sitter_p being NULL; during startup it unconditionally assigns

  *sitter_p = NULL;

but then when it's time to return success, it's conditional like the Unix version:

  if (sitter_p != NULL)
    *sitter_p = sitter;
  else
    _dbus_babysitter_unref (sitter);

D-Bus takes a lot of its coding style conventions from GLib, and one of those conventions is that "out" parameters (using a Foo * parameter to "return" something of type Foo) normally accept NULL for that parameter.
Comment 3 yfei 2016-04-27 15:14:24 UTC
Created attachment 123308 [details] [review]
Fix segfault crash when running test-spawn on Windows.

As suggested, modified _dbus_spawn_async_with_babysitter() in dbus-spawn_win.c to handle sitter_p being NULL.  This is indeed a better way to fix the segfault.
Comment 4 Simon McVittie 2016-04-27 18:23:32 UTC
Comment on attachment 123308 [details] [review]
Fix segfault crash when running test-spawn on Windows.

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

Looks good to me. I'll give Ralf a chance to double-check this.
Comment 5 Ralf Habacker 2016-04-28 17:25:32 UTC
(In reply to Simon McVittie from comment #4)
> Comment on attachment 123308 [details] [review] [review]
> Fix segfault crash when running test-spawn on Windows.
> 
> Review of attachment 123308 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. I'll give Ralf a chance to double-check this.

How is this test invoked ? 

I get a crash running 

xxx@yyy:~/src/dbus-cmake-cross-x86-build> bin/test-spawn.exe bin/test-corrupt.exe

Running the same test on linux returns

#0  0x00007ffff73980a7 in raise () from /lib64/libc.so.6
#1  0x00007ffff7399458 in abort () from /lib64/libc.so.6
#2  0x00007ffff7ba979d in _dbus_abort () at /home/ralf/src/dbus/dbus/dbus-sysdeps.c:91
#3  0x00007ffff7b96a1d in _dbus_real_assert (condition=0, 
    condition_text=0x7ffff7bb73f0 "(error->name != NULL && error->message != NULL) || (error->name == NULL && error->message == NULL)", 
    file=0x7ffff7bb72f0 "/home/ralf/src/dbus/dbus/dbus-errors.c", line=333, func=0x7ffff7bb74d0 <__FUNCTION__.3205> "dbus_error_is_set")
    at /home/ralf/src/dbus/dbus/dbus-internals.c:978
#4  0x00007ffff7b60809 in dbus_error_is_set (error=0x7fffffffd9f0) at /home/ralf/src/dbus/dbus/dbus-errors.c:332
#5  0x0000000000404b95 in _dbus_assert_error_is_clear (error=0x7fffffffd9f0) at /home/ralf/src/dbus/cmake/../dbus/dbus-internals.h:217
#6  0x00000000004064a7 in _dbus_spawn_async_with_babysitter (sitter_p=0x0, log_name=0x7fffffffe00e "bin/test-corrupt", argv=0x613010, env=0x0, 
    child_setup=0x4049ed <setup_func>, user_data=0x0, error=0x7fffffffd9f0) at /home/ralf/src/dbus/dbus/dbus-spawn.c:1232
#7  0x0000000000404aed in main (argc=2, argv=0x7fffffffdb08) at /home/ralf/src/dbus/test/spawn-test.c:33

This is because in dbus-spawn.c line 17 there is an uninitialized instance:

  DBusError error;

which should be 

  DBusError error = DBUS_ERROR_INIT;


Also recent test-spawn test case

  if (!_dbus_spawn_async_with_babysitter (NULL, argv_copy[0], argv_copy, NULL, setup_func, NULL, &error))

sets sitter = NULL so it does not cover changes applied with this patch.
Comment 6 yfei 2016-04-28 17:41:54 UTC
I ran bin\test-spawn.exe bin\test-exit.exe.
Comment 7 Ralf Habacker 2016-04-29 07:51:05 UTC
Created attachment 123339 [details] [review]
Fix assert in test-spawn caused by missing initialization of DBusError instance on gcc builds.

This bug may be hided with msvc debug builds because msvc debug mode initializes all variables.
Comment 8 Simon McVittie 2016-04-29 11:05:31 UTC
(In reply to Ralf Habacker from comment #5)
> in dbus-spawn.c line 17 there is an uninitialized instance:
> 
>   DBusError error;
> 
> which should be 
> 
>   DBusError error = DBUS_ERROR_INIT;

Do you mean spawn-test.c line 19 (as of git master), in main()?

You are right that this is a bug: the error needs to be initialized with either DBUS_ERROR_INIT or dbus_error_init() before first use. Shows how often this bit is tested :-(

(In reply to Ralf Habacker from comment #5)
> How is this test invoked ? 

It isn't run by "make check" or the CMake equivalent. I had previously assumed that it was used indirectly by some other test, like test-segfault is; but git grep doesn't find any references, so I think it might actually be a manual test that we don't ever run in practice.

If it is indeed a manual test, I'd like to rename it to manual-spawn[.c] to go with the other tests that are only run manually.

(In reply to yfei from comment #6)
> I ran bin\test-spawn.exe bin\test-exit.exe.

This seems to be how it's meant to be run - or, more generally, you pass any other program of your choice, and perhaps some command-line arguments. (So you could run "test-spawn.exe notepad c:\windows\system.ini" or something, I think.)
Comment 9 Simon McVittie 2016-04-29 11:05:45 UTC
Comment on attachment 123339 [details] [review]
Fix assert in test-spawn caused by missing initialization of DBusError instance on gcc builds.

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

ship it
Comment 10 Ralf Habacker 2016-04-29 11:09:34 UTC
Comment on attachment 123339 [details] [review]
Fix assert in test-spawn caused by missing initialization of DBusError instance on gcc builds.

committed to master
Comment 11 Simon McVittie 2016-04-29 15:33:57 UTC
Will be fixed in 1.11.4 when released.

Ralf, this can go to the stable branch too if you want (it's a pure bug fix with a small obviously-correct change); but since we don't run this test automatically, maybe it's not worth it. Up to you.
Comment 12 Simon McVittie 2016-04-29 15:34:57 UTC
(Reopening, we haven't applied Attachment #123308 [details] to fix the initially-reported bug yet. Sorry, I obviously need more caffeine.)
Comment 13 Ralf Habacker 2016-04-29 17:17:52 UTC
(In reply to Simon McVittie from comment #12)
> (Reopening, we haven't applied Attachment #123308 [details] to fix the
> initially-reported bug yet. Sorry, I obviously need more caffeine.)

Intended for dbus-1.10 also ?
Comment 14 Ralf Habacker 2016-05-02 05:18:09 UTC
Comment on attachment 123308 [details] [review]
Fix segfault crash when running test-spawn on Windows.

comitted to dbus-1.10 and master because bug has been opened for dbus-1.10
Comment 15 Ralf Habacker 2016-05-02 05:30:08 UTC
(In reply to Simon McVittie from comment #8)
> 
> It isn't run by "make check" or the CMake equivalent. I had previously
> assumed that it was used indirectly by some other test, like test-segfault
> is; but git grep doesn't find any references, so I think it might actually
> be a manual test that we don't ever run in practice.

Why not adding it to make 'check' ?
Comment 16 Simon McVittie 2016-05-02 10:45:17 UTC
(In reply to Ralf Habacker from comment #14)
> comitted to dbus-1.10 and master because bug has been opened for dbus-1.10

That particular patch should be in dbus-1.10, but that wasn't the right logic for why it is OK there. Bug reporters don't get to choose whether a change goes into stable branches, that's part of the maintainer's job; just because a bug *exists* in dbus-1.10 doesn't *necessarily* mean it should be fixed there.

A change should go to the stable branch if it meets all these conditions:

* it fixes a real bug (not a "matter of opinion" like re-indentation)
  This one does: the Windows implementation of
  _dbus_spawn_async_with_babysitter() would crash in a situation where
  the Unix implementation would be fine, and the presence of a test
  indicates that this was meant to work.

* the benefit of merging it (the bug goes away) is significantly more
  than the expected cost (in terms of (risk * severity) of a regression).
  This one is fine: the change is small and I can't see how it could cause
  a regression.

* it is small enough that it isn't going to make it difficult to assess
  the risk of other changes (again, this completely rules out
  re-indentation and most refactoring, for instance).
  This one is fine on this criterion too.

If in doubt, the safe option is to not land changes in stable.
Comment 17 Simon McVittie 2016-05-02 11:23:03 UTC
(In reply to Ralf Habacker from comment #15)
> Why not adding it to make 'check' ?

It doesn't fit the structure of the other automated tests, which are designed to be run with no arguments (in some environment that can be provided by the makefiles), exit 0 on success, and exit nonzero on error.

Adding a shell script that *does* have those properties, or adapting that test to behave that way, would be fine. If it's Unix-only, the test could run /bin/true (exits 0) and /bin/false (exits 1); if it needs to support Windows we'd probably have to build a helper binary that exits 0 and use that.


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.