Bug 83625

Summary: make sure to save and restore errno across _dbus_verbose calls
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: D-Bus Maintainers <dbus>
Severity: normal    
Priority: medium CC: alban.crequy, msniko14, smcv, thiago, walters
Version: 1.5Keywords: patch
Hardware: All   
OS: All   
Whiteboard: review?
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 85108    
Attachments: Consistently save and restore errno

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.