Bug 103010

Summary:	Be more careful with errno in async signal handlers
Product:	dbus	Reporter:	Simon McVittie <smcv>
Component:	core	Assignee:	Simon McVittie <smcv>
Status:	RESOLVED FIXED	QA Contact:	D-Bus Maintainers <dbus>
Severity:	normal
Priority:	medium	Keywords:	patch
Version:	1.10
Hardware:	Other
OS:	All
Whiteboard:	review+
i915 platform:		i915 features:
Attachments:	Make sure non-aborting signal handlers save and restore errno sysdeps: Stop pretending _dbus_set_signal_handler is portable to Windows

Description Simon McVittie 2017-09-27 13:35:33 UTC

Created attachment 134503 [details] [review]
Make sure non-aborting signal handlers save and restore  errno

If an async signal interrupts some function, we can have this
anti-pattern:

    /* in normal code */
    result = some_syscall (); /* fails, e.g. errno = EINVAL */

        /* interrupted by async signal handler */
        write (...); /* fails, e.g. errno = ENOBUFS */

    /* back to normal code */
    if (errno == EINVAL) /* problem! it should be but it isn't */

The solution is for signal handlers to save and restore errno.

This is unnecessary for signal handlers that can't touch errno (like
the one in dbus-launch that just sets a flag), and for signal handlers
that never return (like the one in test-utils-glib for timeouts).

Comment 1 Simon McVittie 2017-09-27 13:36:01 UTC

Created attachment 134504 [details] [review]
sysdeps: Stop pretending _dbus_set_signal_handler is portable to Windows

None of the things we rely on in POSIX async signal handlers, such
as the existence of async-signal-safe write(), are portable to Windows,
so the async signal handlers that use this function are
#ifdef DBUS_UNIX anyway. Remove the unused stub function from the
Windows side, and move the declaration to the Unix-specific header.

Comment 2 Simon McVittie 2017-09-27 13:49:15 UTC

(In reply to Simon McVittie from comment #0)
> Created attachment 134503 [details] [review]
> Make sure non-aborting signal handlers save and restore  errno

Proposed for 1.10

(In reply to Simon McVittie from comment #1)
> Created attachment 134504 [details] [review]
> sysdeps: Stop pretending _dbus_set_signal_handler is portable to Windows

Not for 1.10

Comment 3 Philip Withnall 2017-09-27 13:53:25 UTC

Comment on attachment 134503 [details] [review]
Make sure non-aborting signal handlers save and restore  errno

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

++

::: dbus/dbus-spawn.c
@@ +1146,4 @@
>   again:
>    if (write (babysit_sigchld_pipe, &b, 1) <= 0) 
>      if (errno == EINTR)
>        goto again;

Christ, this is a horrific way of doing a loop. However, ENOTYOURPROBLEM.

Comment 4 Philip Withnall 2017-09-27 13:53:58 UTC

Comment on attachment 134504 [details] [review]
sysdeps: Stop pretending _dbus_set_signal_handler is portable to Windows

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

Seems sensible.

Comment 5 Simon McVittie 2017-09-27 14:33:18 UTC

Thanks, pushed for 1.11.20. I might backport the first patch to 1.10.x at some point but not right now - nobody has noticed this being a problem so it's probably 99% theoretical.

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.