Bug 103010 - Be more careful with errno in async signal handlers
Summary: Be more careful with errno in async signal handlers
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.10
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-09-27 13:35 UTC by Simon McVittie
Modified: 2017-09-27 14:33 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Make sure non-aborting signal handlers save and restore errno (2.16 KB, patch)
2017-09-27 13:35 UTC, Simon McVittie
Details | Splinter Review
sysdeps: Stop pretending _dbus_set_signal_handler is portable to Windows (2.57 KB, patch)
2017-09-27 13:36 UTC, Simon McVittie
Details | Splinter Review

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.