Bug 95155 - test-bus unit test fails at check_segfault_no_auto_start test
Summary: test-bus unit test fails at check_segfault_no_auto_start test
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 15:44 UTC by yfei
Modified: 2016-05-09 19:55 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
test-bus verbose output log (439.23 KB, text/plain)
2016-04-26 15:44 UTC, yfei
Details
Patch to disable Windows popup when test-segfault.exe crashes. (1.97 KB, patch)
2016-04-26 21:15 UTC, yfei
Details | Splinter Review
Patch to disable Windows popup when test-segfault.exe crashes. (1.47 KB, patch)
2016-04-27 14:49 UTC, yfei
Details | Splinter Review
Suppress Windows popups and jit debugger when app crashes with exception. (1.47 KB, patch)
2016-05-09 15:29 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 15:44:36 UTC
Created attachment 123278 [details]
test-bus verbose output log

The test-bus unit test on Windows systems fails during the check_segfault_no_auto_start test.  The full verbose output from running test-bus.exe is attached.  As you can see, while attempting to activate the test-segfault application, dbus receives DBUS_ERROR_TIMED_OUT. I think the reason for the timeout failure is because dbus is waiting for the child process to exit to detect any application failures based on the exit code.  However, on Windows, when a segmentation fault occurs, the OS presents a pop-up window to the user, and the application does not exit until the user manually dismisses the popup dialog box.  If I dismiss the dialogs boxes before service_start_timeout expires, then dbus correctly reports the child process exited with exception C0000005.  It seems like the following change should be made to dispatch.c:

diff --git a/bus/dispatch.c b/bus/dispatch.c
index edfa1b4..680cad3 100644
--- a/bus/dispatch.c
+++ b/bus/dispatch.c
@@ -3026,6 +3026,11 @@ check_segfault_service_no_auto_start (BusContext     *context,
         {
           /* unhandled exceptions are normal exit codes */
         }
+      else if (dbus_message_is_error (message,
+                                      DBUS_ERROR_TIMED_OUT))
+        {
+          /* If a service quits unexpectedly, a time out error is expected */
+        }
 #else
       else if (dbus_message_is_error (message,
                                       DBUS_ERROR_SPAWN_CHILD_SIGNALED))
Comment 1 Simon McVittie 2016-04-26 17:14:28 UTC
(In reply to yfei from comment #0)
> I think the
> reason for the timeout failure is because dbus is waiting for the child
> process to exit to detect any application failures based on the exit code. 
> However, on Windows, when a segmentation fault occurs, the OS presents a
> pop-up window to the user, and the application does not exit until the user
> manually dismisses the popup dialog box.

Is there any way an application can ask to suppress this crash handler, like the way we suppress core dumps using prctl() on Linux? If there is, test-segfault should do it.
Comment 2 Simon McVittie 2016-04-26 17:16:16 UTC
Looks like https://blogs.msdn.microsoft.com/oldnewthing/20040727-00/?p=38323/ might be the right runes to use.
Comment 3 yfei 2016-04-26 17:42:35 UTC
Tested that change and it works.  Here is are the changes to suppress the Windows popup:

diff --git a/test/test-segfault.c b/test/test-segfault.c
index c062ce1..43d810c 100644
--- a/test/test-segfault.c
+++ b/test/test-segfault.c
@@ -1,4 +1,7 @@
 /* This is simply a process that segfaults */
+#ifdef DBUS_WIN
+#include <windows.h>
+#endif
 #include <config.h>
 #include <stdlib.h>
 #ifdef HAVE_SIGNAL_H
@@ -18,6 +21,15 @@ main (int argc, char **argv)
 {
   char *p;  
 
+#ifdef DBUS_WIN
+  // Diable the Windows popup dialog when an app crashes so that we don't
+  // wait for a user to manually dismiss the dialog before the app quits
+  // with an erorr code.
+  // See https://blogs.msdn.microsoft.com/oldnewthing/20040727-00/?p=38323/
+  DWORD dwMode = SetErrorMode(SEM_NOGPFAULTERRORBOX);
+  SetErrorMode(dwMode | SEM_NOGPFAULTERRORBOX);
+#endif 
+
 #if HAVE_SETRLIMIT
   /* No core dumps please, we know we crashed. */
   struct rlimit r = { 0, };
Comment 4 Ralf Habacker 2016-04-26 18:16:25 UTC
(In reply to yfei from comment #3)
> Tested that change and it works.  Here is are the changes to suppress the
> Windows popup:
> 

Do not work with recent wine 1.9.7, where I get 
Activated service 'org.freedesktop.DBus.TestSuiteSegfaultService' failed: Process org.freedesktop.DBus.TestSuiteSegfaultService exited with status -1073741819
Activating service name='org.freedesktop.DBus.TestSuiteSegfaultService'
wine: Unhandled page fault on write access to 0x00000000 at address 0x4015a4 (thread 0029), starting debugger...
fixme:console:CONSOLE_DefaultHandler Terminating process 32 on event 0

I need to use SetUnhandledExceptionFilter() from http://stackoverflow.com/questions/396369/how-do-i-disable-the-debug-close-application-dialog-on-windows-vista
Comment 5 yfei 2016-04-26 18:48:05 UTC
Test with additional call to SetUnhandledExceptionFilter() on Win10 x64 and it works well.
Comment 6 Simon McVittie 2016-04-26 20:12:50 UTC
(In reply to yfei from comment #5)
> Test with additional call to SetUnhandledExceptionFilter() on Win10 x64 and
> it works well.

Please attach the exact patch that you've successfully tested, in "git format-patch" format so we can credit you correctly.
Comment 7 Simon McVittie 2016-04-26 20:16:48 UTC
Review:

(In reply to yfei from comment #3)
> +#ifdef DBUS_WIN
> +#include <windows.h>
> +#endif
>  #include <config.h>

#include <config.h> must be the very first thing in the file, apart from perhaps comments. Please move this down to below the Standard C headers like stdlib.h. If there are other #ifdef'd blocks of #include directives, for example for the Linux prctl thing, next to them would be a good place to put this.

> +#ifdef DBUS_WIN
> +  // Diable the Windows popup dialog when an app crashes so that we don't
> +  // wait for a user to manually dismiss the dialog before the app quits
> +  // with an erorr code.
> +  // See https://blogs.msdn.microsoft.com/oldnewthing/20040727-00/?p=38323/
> +  DWORD dwMode = SetErrorMode(SEM_NOGPFAULTERRORBOX);
> +  SetErrorMode(dwMode | SEM_NOGPFAULTERRORBOX);
> +#endif 

D-Bus is C89, not C++ or C99, so please use /* these comments */.

Spelling: "Disable", "error".

You don't necessarily need to put the URL reference in the implementation, but it would make sense to mention it in the commit message, along with the URL of this bug.
Comment 8 yfei 2016-04-26 21:15:45 UTC
Created attachment 123282 [details] [review]
Patch to disable Windows popup when test-segfault.exe crashes.
Comment 9 Ralf Habacker 2016-04-27 06:17:12 UTC
Comment on attachment 123282 [details] [review]
Patch to disable Windows popup when test-segfault.exe crashes.

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

::: test/test-segfault.c
@@ +26,5 @@
> +}
> +
> +int
> +runtime_check_handler(int           errorType,
> +                      const char    *filename,

should be be removed because it is required by _RTC_SetErrorFunc(),which is not needed

@@ +54,5 @@
> +   * the dialog.  */
> +  DWORD dwMode = SetErrorMode(SEM_NOGPFAULTERRORBOX);
> +  SetErrorMode(dwMode | SEM_NOGPFAULTERRORBOX);
> +  SetUnhandledExceptionFilter((LPTOP_LEVEL_EXCEPTION_FILTER)&exception_handler);
> +  _RTC_SetErrorFunc(&runtime_check_handler);

This one is not needed and should be removed.
Comment 10 yfei 2016-04-27 14:49:05 UTC
Created attachment 123307 [details] [review]
Patch to disable Windows popup when test-segfault.exe crashes.

Removed unnecessary custom RTC handler of the first proposed patch.
Comment 11 yfei 2016-05-05 22:51:59 UTC
Hi Ralf, does the new patch look ok?
Comment 12 Ralf Habacker 2016-05-07 06:41:50 UTC
(In reply to yfei from comment #11)
> Hi Ralf, does the new patch look ok?

I'm coming back to this next week.
Comment 13 Ralf Habacker 2016-05-09 12:33:23 UTC
(In reply to Ralf Habacker from comment #12)
> (In reply to yfei from comment #11)
> > Hi Ralf, does the new patch look ok?
> 
> I'm coming back to this next week.


with the patch applied to git master I did run 'test-bus --tap dispatch' from build dir and with related env set, which returns 
... 
Activating service name='org.freedesktop.DBus.TestSuiteEchoService'
Successfully activated service 'org.freedesktop.DBus.TestSuiteEchoService'
Activating service name='org.freedesktop.DBus.TestSuiteSegfaultService'
Exception detected during the unit tests!
Activated service 'org.freedesktop.DBus.TestSuiteSegfaultService' failed: Process org.freedesktop.DBus.TestSuiteSegfaultService exited with status 1
... 

> Exception detected during the unit tests!

-> This shows that the exceptions has been raised, which is good

Unfortunally the message do noit giva any hint of the trigger source. I suggest to change the message to 

test-segfault: Exception detected

and to let it print to stderr (currently stdout) or to remove it complety. 

@Simon: What do you think ?  

> Activated service 'org.freedesktop.DBus.TestSuiteSegfaultService' failed: Process org.freedesktop.DBus.TestSuiteSegfaultService exited with status 1

The q

@Simon:any idea how to hide it ?
Comment 14 Ralf Habacker 2016-05-09 12:34:33 UTC
sed 's/The q/The question is, if this error code is okay for returning on handled exception/,g'
Comment 15 Simon McVittie 2016-05-09 14:45:27 UTC
(In reply to Ralf Habacker from comment #13)
> > Exception detected during the unit tests!
> 
> -> This shows that the exceptions has been raised, which is good
> 
> Unfortunally the message do noit giva any hint of the trigger source. I
> suggest to change the message to 
> 
> test-segfault: Exception detected

Or in fact, since the entire point of this executable is that it segfaults (it is there to test what happens when dbus-daemon tries to activate a service and the service just crashes), it should either print something like

test-segfault: raised fatal exception as intended

or remain silent.

> and to let it print to stderr (currently stdout) or to remove it complety. 

If it prints anything at all, it should be to stderr. stdout could interfere with parsing the TAP-format output of the tests.

> > Activated service 'org.freedesktop.DBus.TestSuiteSegfaultService' failed: Process org.freedesktop.DBus.TestSuiteSegfaultService exited with status 1
>
> @Simon:any idea how to hide it ?

Don't hide it. This is the dbus-daemon code behaving as intended: it is meant to log to stderr when an activated service fails, and it is doing so, so that's good.

(In reply to Ralf Habacker from comment #14)
> The question is, if this error code is okay for returning on
> handled exception

If there is an exit status that is traditional for processes crashing with a segmentation fault (access violation, general protection fault) on Windows, exit with that status. From Comment #0 and a quick web search, it looks as though 0xC0000005 might be the most appropriate exit status?

(The equivalent on Linux would be exit status 139, which is how Unix shells traditionally represent SIGSEGV in the "$?" special variable.)

If there is no traditional exit status for segfaulting processes, anything nonzero is fine.
Comment 16 Ralf Habacker 2016-05-09 15:29:13 UTC
Created attachment 123573 [details] [review]
Suppress Windows popups and jit debugger when app crashes with exception.

Bug:
Comment 17 Ralf Habacker 2016-05-09 15:30:12 UTC
Comment on attachment 123307 [details] [review]
Patch to disable Windows popup when test-segfault.exe crashes.

superseeded
Comment 18 Ralf Habacker 2016-05-09 15:33:24 UTC
(In reply to Simon McVittie from comment #15)
> (In reply to Ralf Habacker from comment #13)
> > > Exception detected during the unit tests!
> > 
> > -> This shows that the exceptions has been raised, which is good
> > 
> > Unfortunally the message do noit giva any hint of the trigger source. I
> > suggest to change the message to 
> > 
> > test-segfault: Exception detected
> 
> Or in fact, since the entire point of this executable is that it segfaults
> (it is there to test what happens when dbus-daemon tries to activate a
> service and the service just crashes), it should either print something like
> 
> test-segfault: raised fatal exception as intended
fixed
> 
> > and to let it print to stderr (currently stdout) or to remove it complety. 
> 
redirected to stderr
> If it prints anything at all, it should be to stderr. stdout could interfere
> with parsing the TAP-format output of the tests.
fixed
> > > Activated service 'org.freedesktop.DBus.TestSuiteSegfaultService' failed: Process org.freedesktop.DBus.TestSuiteSegfaultService exited with status 1

> Don't hide it. This is the dbus-daemon code behaving as intended: it is
> meant to log to stderr when an activated service fails, and it is doing so,
> so that's good.
okay

> (In reply to Ralf Habacker from comment #14)
> > The question is, if this error code is okay for returning on
> > handled exception
> 
> If there is an exit status that is traditional for processes crashing with a
> segmentation fault (access violation, general protection fault) on Windows,
> exit with that status. From Comment #0 and a quick web search, it looks as
> though 0xC0000005 might be the most appropriate exit status?
Correct, but we cannot use exit(), because it returns only the lower byte to the parent (see https://msdn.microsoft.com/de-de/library/6wdz5232.aspx)

It is required to use ExitProcess() https://msdn.microsoft.com/en-us/library/windows/desktop/ms682658(v=vs.85).aspx

fixed in updated patch
Comment 19 Simon McVittie 2016-05-09 17:55:55 UTC
Comment on attachment 123573 [details] [review]
Suppress Windows popups and jit debugger when app crashes with exception.

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

Please say "Based on a patch from Yiyang Fei" in the long commit message - we should credit our contributors.

Other than that, looks great. Please commit to master with that change.

This would also be fine in dbus-1.10, if you want; up to you.
Comment 20 Ralf Habacker 2016-05-09 19:49:27 UTC
Comment on attachment 123573 [details] [review]
Suppress Windows popups and jit debugger when app crashes with exception.

committed to dbus-1.10 and master with original author added to commit message.
Comment 21 yfei 2016-05-09 19:55:16 UTC
Thank you!


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.