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))
(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.
Looks like https://blogs.msdn.microsoft.com/oldnewthing/20040727-00/?p=38323/ might be the right runes to use.
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, };
(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
Test with additional call to SetUnhandledExceptionFilter() on Win10 x64 and it works well.
(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.
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.
Created attachment 123282 [details] [review] Patch to disable Windows popup when test-segfault.exe crashes.
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.
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.
Hi Ralf, does the new patch look ok?
(In reply to yfei from comment #11) > Hi Ralf, does the new patch look ok? I'm coming back to this next week.
(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 ?
sed 's/The q/The question is, if this error code is okay for returning on handled exception/,g'
(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.
Created attachment 123573 [details] [review] Suppress Windows popups and jit debugger when app crashes with exception. Bug:
Comment on attachment 123307 [details] [review] Patch to disable Windows popup when test-segfault.exe crashes. superseeded
(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 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 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.
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.