Bug 83625 - make sure to save and restore errno across _dbus_verbose calls
Summary: make sure to save and restore errno across _dbus_verbose calls
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks: 85108
  Show dependency treegraph
 
Reported: 2014-09-08 19:33 UTC by Simon McVittie
Modified: 2014-10-29 16:22 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Consistently save and restore errno (16.05 KB, patch)
2014-09-08 19:33 UTC, Simon McVittie
Details | Splinter Review

Description Simon McVittie 2014-09-08 19:33:58 UTC
Created attachment 105916 [details] [review]
Consistently save and restore errno

Some functions in dbus-transport-socket.c make a (wrapped)
socket syscall, then call other APIs, then test the result and
errno of the socket syscall.
    
This would break horribly if those "other APIs" overwrote errno with
their own value (... and this is part of why errno is an awful API).

Notably, if running under DBUS_VERBOSE, _dbus_verbose() is basically
fprintf(), which sets errno; and our Unix fd-passing support
makes calls of the form _dbus_verbose ("Read/wrote %i unix fds\n", n)
between the syscall and the result processing.
    
Maybe one day we'll convert all of dbus' syscall wrappers to either
raise a DBusError, or use the "negative errno" convention that systemd
borrowed from the Linux kernel, and in particular, we would need to
do that if we ever ported it to a platform where socket error reporting
was not basically errno. However, in practice everyone uses something
derived from BSD sockets, so "this sets errno, you know what errno is"
is a good enough internal API if we make sure to use it correctly.
    
Nothing calls _dbus_get_is_errno_nonzero(), so I just removed it instead
of converting it to the new calling convention.

---

As well as reviews, I'd appreciate maintainer opinions on whether this should go to the 1.8 or master branch. It's a bugfix, but we've never actually observed the bug, and the fix is intrusive; my personal opinion would be 1.9.
Comment 1 Simon McVittie 2014-10-09 10:43:17 UTC
This has now been here for a month with no objections. If nobody reviews it in the next 2 weeks, I intend to go for "easier to ask forgiveness than permission" and commit it to 1.9.
Comment 2 Simon McVittie 2014-10-29 16:22:06 UTC
(In reply to Simon McVittie from comment #1)
> This has now been here for a month with no objections. If nobody reviews it
> in the next 2 weeks, I intend to go for "easier to ask forgiveness than
> permission" and commit it to 1.9.

Done. Fixed in git for 1.9.2


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.