Bug 74029 - init_session_address() should raise a DBusError instead of doing _dbus_warn()
Summary: init_session_address() should raise a DBusError instead of doing _dbus_warn()
Status: RESOLVED MOVED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review-, Mac testing also needed
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-24 19:33 UTC by Guy Harris
Modified: 2018-10-12 21:17 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH] Mac: do not warn and ignore error for init session bus address (3.05 KB, patch)
2014-02-11 05:12 UTC, Chengwei Yang
Details | Splinter Review

Description Guy Harris 2014-01-24 19:33:05 UTC
Bug 6964 against Wireshark:

    https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=9694

is due to:

    the person using the MacPorts version of Wireshark, which uses the MacPorts version of libpcap;

    the MacPorts version of libpcap being a recent version, with support for capturing D-Bus traffic, and built with D-Bus support;

    libpcap refusing to provide, in its API to get a list of devices on which it can capture, devices that can't be opened, and testing that by trying to open them;

    calls to try to open the "dbus-system" device calling dbus_bus_get with DBUS_BUS_SYSTEM as the first argument;

    the system perhaps not being configured properly;

    Wireshark running its dumpcap program to get a list of devices on which it can capture (so that, if special privileges are needed to open devices on which to capture traffic, only dumpcap needs them, not Wireshark in its entirety), with the standard output and error from dumpcap piped to it, and assuming that only serious "I can't function" errors will show up on the standard error, in a standard format determined by dumpcap (failure to open a device is *not* such an error, under any circumstances, not even failure due to org.freedesktop.dbus-session.plist not having been loaded on OS X);

    init_session_address() calling _dbus_warn() if _dbus_lookup_session_address() sets "supported" to TRUE and returns FALSE, causing exactly such an error to go to the standard error.

I didn't find any obvious way to suppress the error message, so I'm probably just going to forcibly disable D-Bus support on OS X in libpcap, pending some way to suppress it.

It's probably best if library routines not write to the standard output or error except in calls whose purpose is to write to the standard output or error.  (And, yes, I know libpcap does that in some places; I'll fix that soon.)
Comment 1 Simon McVittie 2014-01-27 11:02:06 UTC
(In reply to comment #0)
>     calls to try to open the "dbus-system" device calling dbus_bus_get with
> DBUS_BUS_SYSTEM as the first argument;
> 
>     the system perhaps not being configured properly;

This is probably not appropriate to do by default on Mac OS, since the operating system as shipped by its vendor (i.e. Apple) doesn't have a system bus. Eavesdropping on the D-Bus system bus doesn't work without intrusive and insecure reconfiguration, so I would suggest having "list my devices" API calls in pcap/wireshark not try that.

Having said that, if you used dbus_bus_get(DBUS_BUS_SESSION, .) the same code would run, as far as I can see.

It seems somewhat inappropriate that libdbus' Mac OS launchd support is behaving like this: given that it can fail, I'd somewhat prefer it if it used a pseudo-protocol (maybe "launchd:"?) in the same way as X11's "autolaunch:", which runs at a stage in the connection process that *can* handle errors.

> assuming that only serious "I can't function" errors will show up on the
> standard error

This is not a safe assumption for generic libraries. stderr is the only universal fallback for handling serious errors, unfortunately.

However, libdbus is not meant to log to stderr unless:

* a caller is using it wrong ("programmer error" that cannot be corrected at runtime, only by changing the caller's source code); or
* it has detected internal breakage (assertion failure)

>     init_session_address() calling _dbus_warn() if
> _dbus_lookup_session_address() sets "supported" to TRUE and returns FALSE,
> causing exactly such an error to go to the standard error.

It looks as though the bug here is that init_session_address() and  init_connections_unlocked() have no better way to report an error. They should use DBusError for structured error-reporting instead, like internal_bus_get() does.

Patches welcome... (I'd write one myself, but then I wouldn't be able to review it, and it'd sit in Bugzilla forever.)
Comment 2 Chengwei Yang 2014-02-11 05:12:04 UTC
Created attachment 93825 [details] [review]
[PATCH] Mac: do not warn and ignore error for init session bus  address

This is not a major bug but does cause some inconveniences for lib
users, it's better we can return an error rather than print to stderr
and ignore the error.

I didn't verify this patch but pass "make check" since I have no Mac machine.
Comment 3 Simon McVittie 2014-04-28 14:39:47 UTC
Comment on attachment 93825 [details] [review]
[PATCH] Mac: do not warn and ignore error for init session bus  address

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

> I didn't verify this patch but pass "make check" since I have no Mac machine.

Regardless of any reviews, I won't merge this patch until it has been tested in the situation it is intended to fix.

::: dbus/dbus-bus.c
@@ -159,4 @@
>        /* So it's not in the environment - let's try a platform-specific method.
>         * On MacOS, this involves asking launchd.  On Windows (not specified yet)
>         * we might do a COM lookup.
> -       * Ignore errors - if we failed, fall back to autolaunch. */

Is there any platform where supported will be set to TRUE, an error might occur, but autolaunch might later turn out to work? If so, you've just broken it.

@@ +434,3 @@
>      {
> +      if (!dbus_error_is_set (error))
> +        _DBUS_SET_OOM (error);

If init_connections_unlocked() takes a DBusError parameter, then it should always be responsible for filling it - in particular, it should call _DBUS_SET_OOM() where appropriate.

DBusError is meant to be like GLib's GError, see the GLib documentation for policies and conventions.

Think of returning FALSE with the error set as being like throwing an exception in higher-level languages.
Comment 4 Simon McVittie 2014-09-23 14:12:02 UTC
undoing abusive changes
Comment 5 GitLab Migration User 2018-10-12 21:17:23 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/94.


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.