Bug 104646 - org.bluez.MediaEndpoint1.Release should do nothing, not raise a DBUS error
Summary: org.bluez.MediaEndpoint1.Release should do nothing, not raise a DBUS error
Status: RESOLVED FIXED
Alias: None
Product: PulseAudio
Classification: Unclassified
Component: modules (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: pulseaudio-bugs
QA Contact: pulseaudio-bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-15 19:38 UTC by faminebadger
Modified: 2019-05-09 11:36 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
0001-bluetooth-don-t-send-unsolicted-replies-to-the-endpo.patch (2.72 KB, patch)
2018-01-16 21:53 UTC, Tanu Kaskinen
Details | Splinter Review

Description faminebadger 2018-01-15 19:38:31 UTC
The endpoint_release function (bluez5-util.c) currently just returns an Unimplemented error message, which on a standard DBUS setup gets blocked and generates an unwanted entry in syslog - e.g.:

> Jan 14 04:51:32 gentoo dbus-daemon[2492]: [system] Rejected send message, 2 matched rules; type="error", sender=":1.15" (uid=1000 pid=2864 comm="/usr/bin/pulseaudio --start --log-target=syslog ") interface="(unset)" member="(unset)" error name="org.bluez.MediaEndpoint1.Error.NotImplemented" requested_reply="0" destination=":1.1" (uid=0 pid=2670 comm="/usr/libexec/bluetooth/bluetoothd ")
> Jan 15 06:40:08 gentoo dbus-daemon[2512]: [system] Rejected send message, 2 matched rules; type="error", sender=":1.16" (uid=1000 pid=2866 comm="/usr/bin/pulseaudio --start --log-target=syslog ") interface="(unset)" member="(unset)" error name="org.bluez.MediaEndpoint1.Error.NotImplemented" requested_reply="0" destination=":1.0" (uid=0 pid=2596 comm="/usr/libexec/bluetooth/bluetoothd ")
> Jan 15 06:40:08 gentoo dbus-daemon[2512]: [system] Rejected send message, 2 matched rules; type="error", sender=":1.16" (uid=1000 pid=2866 comm="/usr/bin/pulseaudio --start --log-target=syslog ") interface="(unset)" member="(unset)" error name="org.bluez.MediaEndpoint1.Error.NotImplemented" requested_reply="0" destination=":1.0" (uid=0 pid=2596 comm="/usr/libexec/bluetooth/bluetoothd ")
> Jan 15 06:40:08 gentoo dbus-daemon[2512]: [system] Rejected send message, 2 matched rules; type="error", sender=":1.16" (uid=1000 pid=2866 comm="/usr/bin/pulseaudio --start --log-target=syslog ") interface="(unset)" member="(unset)" error name="org.bluez.MediaEndpoint1.Error.NotImplemented" requested_reply="0" destination=":1.0" (uid=0 pid=2596 comm="/usr/libexec/bluetooth/bluetoothd ")
> Jan 15 06:40:08 gentoo dbus-daemon[2512]: [system] Rejected send message, 2 matched rules; type="error", sender=":1.16" (uid=1000 pid=2866 comm="/usr/bin/pulseaudio --start --log-target=syslog ") interface="(unset)" member="(unset)" error name="org.bluez.MediaEndpoint1.Error.NotImplemented" requested_reply="0" destination=":1.0" (uid=0 pid=2596 comm="/usr/libexec/bluetooth/bluetoothd ")

And so on...  I get this message multiple times per day, and it causes logcheck to spam me with mail.

Given the documentation for this function (see https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/media-api.txt) says this is just a cleanup call post unregister to notify the endpoint for cleanup, if there is no cleanup required it seems more correct to do nothing and return success than do nothing and return an error.
Comment 1 faminebadger 2018-01-15 19:39:27 UTC
Crap I misspelled DBUS as DUS in the title - oops.  Can't edit that now myself, but someone can change it if they feel the urge.
Comment 2 Tanu Kaskinen 2018-01-15 23:19:36 UTC
I agree that we shouldn't respond with an error, but I believe that's not enough to make the log spam go away. The D-Bus daemon should treat both success and failure replies in the same way. I think the relevant thing is that bluetoothd didn't request a reply to the Release() call, and the D-Bus daemon is not allowing any replies in such case.

Are you able to test a patch if I write one?
Comment 3 faminebadger 2018-01-16 01:33:15 UTC
I'm not really very familiar with DBus, so I hadn't considered that it might just be because the error was unsolicited (I thought perhaps an exception was never a requested reply), but yes that sounds reasonable.

I can test a patch quite easily - a fresh reboot reliably produces the message for me.
Comment 4 Tanu Kaskinen 2018-01-16 21:53:03 UTC
Created attachment 136793 [details] [review]
0001-bluetooth-don-t-send-unsolicted-replies-to-the-endpo.patch

Thanks for the offer to test! Bluetooth works so badly and inconsistently on my machine that I dislike the idea of testing anything bluetooth related myself.

A patch is attached.
Comment 5 faminebadger 2018-01-16 23:37:27 UTC
Yes, that seems to work.

However, I think it's not the best design to check dbus_message_get_no_reply in endpoint_release.  That means there will be dozens of other places that construct dbus responses that remain unprotected.

I think it preferable to find a place higher up that call tree that is common to all dbus message handlers (or at least all bluez ones) that can check the flag and destroy any unsolicited message that has been constructed and returned by the individual handlers.

That would fix this, but also every other possible instance.
Comment 6 Tanu Kaskinen 2018-01-18 20:56:13 UTC
Thanks for testing, I submitted the patch to the mailing list now.

It's a valid point that it's very easy to forget to check the no_reply flag if the checking isn't centralized. That said, it's also very rare that a method call doesn't expect a reply. I'm not motivated enough to work on this.

If someone else wants to work on it, here are my thoughts about the possible implementation:

Ideally libdbus would implement the checking in dbus_connection_send(). Maybe it can't be changed to do that unconditionally, but applications could set an option in the DBusConnection object if they want to enable the filtering.

If the libdbus maintainers reject the feature, then we can do the filtering in PulseAudio, although I don't see any pretty solutions. I can think of two not-so-pretty solutions:

1) Ban the direct use of dbus_connection_send(), and use a wrapper function instead. The wrapper function would take a boolean argument indicating whether or not to actually send the message. That would force every caller to think about whether or not the message should be sent. It would be nice if the send wrapper could get the no_reply flag directly from the message that is being sent, so that the explicit boolean argument wouldn't be needed, but libdbus doesn't seem to provide a function for getting the "in-reply-to" message from the reply message. That feature might be feasible to get added to libdbus, though.

2) Ban the direct use of dbus_message_new_method_return(), dbus_message_new_error() and dbus_message_new_error_printf(), and use wrapper functions instead. The wrapper functions would fail if the original method call didn't request a reply.
Comment 7 Tanu Kaskinen 2018-01-20 22:23:09 UTC
The patch was applied, so I'm closing this bug.


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.