I am able to kill the user's session bus reproducible by running the following command line: dbus-monitor --profile "interface='org.kde.walletd',member='/modules/kwalletd/org.kde.KWallet/walletOpened'" in a KDE session (mostly KDE4/Plasma 4) under Arch Linux. Program 'dbus-monitor' is part of Arch Linux's package 'core/dbus' which is installed in version 1.10.0-3. A bug has been reported in Arch Linux's bug databse (FS#46535). The command line is most likely wrong, but DBus should still not crash just because of wrongful parameters. Once the session DBus has crash, the desktop environment is severely affected, such as that one can no longer log out properly using Plasma widgets (e.g. K Menu). My work-around has been to switch to text mode (Ctrl+Alt+Fn), log in as root, and restart the kdm service. Please test if you can reproduce this crash. Unfortunately, Arch Linux does not provide debug packages and my attempts to attach gdb to the dbus session bus process right before triggering the crash have not revealed any information (not an expert here). Please let me know how to proceed.
Please recompile dbus in debug mode or at the very least with -rdynamic and no symbol stripping. A backtrace (by anyone) would be most appreciated. I cannot reproduce this issue using 1.18.16. The daemon replies with error "org.freedesktop.DBus.Error.MatchRuleInvalid" and message "Member name '/modules/kwalletd/org.kde.KWallet/walletOpened' is invalid"
This is a bug in the new "monitoring" code, which mostly supersedes the old "eavesdropping" in D-Bus 1.10. (Bug #46787)
Created attachment 118708 [details] [review] BecomeMonitor: do not overwrite error with another error If the user gave us a syntactically invalid error name, we'd overwrite the MatchRuleInvalid error with NoMemory, causing an assertion failure (crash) in the dbus-daemon. This is not a denial-of-service vulnerability on the system bus, because monitoring is a privileged action, and root privilege is checked before this code is reached. However, it's an annoying bug on the session bus.
Created attachment 118709 [details] [review] Add a regression test for invalid BecomeMonitor calls --- This reproduces a dbus-daemon crash, unless Attachment #118708 [details] is applied first. This is a simplified version of the bug reporter's steps-to-reproduce. I also added a test for passing nonzero flags to BecomeMonitor, since that's the other major way for the method implementation to fail (it takes a flags parameter, but no flags are defined yet, so nonzero flags are always InvalidArgument).
Created attachment 118710 [details] [review] Add a regression test for invalid BecomeMonitor calls --- Superseding previous patch with a couple of minor fixes: * use the same syntactically invalid match rule that the bug reporter quoted * remove a leftover call to dbus_error_free()
(In reply to Thomas from comment #0) > The command line is most likely wrong Yes: I think you said "member" when you meant "path". A member is a method or signal name, like GetNameOwner or NameOwnerChanged. > but DBus should still not crash just because of wrongful parameters Indeed. I believe the first patch on this bug should fix it. If this had also affected the system bus, it would have been a minor security vulnerability (denial of service); but as far as I can see, it doesn't.
Any reviews on this? I'd like to get this into 1.10.2 fairly soon, and the change is minimal, so I'll merge this unreviewed if there's no feedback.
Comment on attachment 118708 [details] [review] BecomeMonitor: do not overwrite error with another error Review of attachment 118708 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 118710 [details] [review] Add a regression test for invalid BecomeMonitor calls Review of attachment 118710 [details] [review]: ----------------------------------------------------------------- Running this test after adding the missing " on the end of line 592 returns on windows using wine: /monitor/invalid: ** ERROR:../../dbus/test/monitor.c:572:test_invalid: assertion failed (dbus_message_get_error_name (m) == DBUS_ERROR_INVALID_ARGS): ("org.freedesktop.DBus.Error.UnknownInterface" == "org.freedesktop.DBus.Error.InvalidArgs") abnormal program termination Is this an expected test result ? ::: test/monitor.c @@ +588,5 @@ > + if (!dbus_message_iter_open_container (&appender, DBUS_TYPE_ARRAY, "s", > + &array_appender)) > + g_error ("OOM"); > + > + s = "interface='org.kde.walletd',member='/modules/kwalletd/org.kde.KWallet/walletOpened'; ../../dbus/test/monitor.c: In function 'test_invalid': ../../dbus/test/monitor.c:592:7: warning: missing terminating " character s = "interface='org.kde.walletd',member='/modules/kwalletd/org.kde.KWallet/walletOpened'; ^ ../../dbus/test/monitor.c:592:3: error: missing terminating " character s = "interface='org.kde.walletd',member='/modules/kwalletd/org.kde.KWallet/walletOpened'; ^ ../../dbus/test/monitor.c:594:3: error: expected expression before 'if' if (!dbus_message_iter_append_basic (&array_appender, DBUS_TYPE_STRING, ^
Comment on attachment 118710 [details] [review] Add a regression test for invalid BecomeMonitor calls Review of attachment 118710 [details] [review]: ----------------------------------------------------------------- ::: test/monitor.c @@ +546,5 @@ > + g_error ("OOM"); > + > + if (!dbus_message_iter_close_container (&appender, &array_appender) || > + !dbus_message_iter_append_basic (&appender, DBUS_TYPE_UINT32, > + &invalid_flags)) You could combine these two `if` statements. @@ +595,5 @@ > + &s)) > + g_error ("OOM"); > + > + if (!dbus_message_iter_close_container (&appender, &array_appender) || > + !dbus_message_iter_append_basic (&appender, DBUS_TYPE_UINT32, &zero)) Could combine these two too.
Comment on attachment 118708 [details] [review] BecomeMonitor: do not overwrite error with another error Review of attachment 118708 [details] [review]: ----------------------------------------------------------------- Looks good to me.
(In reply to Ralf Habacker from comment #9) > ../../dbus/test/monitor.c:592:7: warning: missing terminating " character Oops, yes. I thought I'd tested this version of the patch, but apparently I didn't. > (dbus_message_get_error_name (m) == DBUS_ERROR_INVALID_ARGS): > ("org.freedesktop.DBus.Error.UnknownInterface" == > "org.freedesktop.DBus.Error.InvalidArgs") ... > Is this an expected test result ? No, this is a test failure. However, it doesn't indicate that the patch is wrong, either; it indicates that you are testing against a pre-1.10 dbus-daemon that didn't have the new monitoring API. I think this is because some of the GLib-based tests default to using "autolaunch:" which is global to a machine. I'll open a separate bug for these tests not working correctly in Windows builds and under Wine.
(In reply to Simon McVittie from comment #12) > I'll open a separate bug for these tests not working correctly in Windows > builds and under Wine. Bug #92538
Comment on attachment 118708 [details] [review] BecomeMonitor: do not overwrite error with another error Pushed to dbus-1.10. Fixed in git for 1.10.2 and 1.11.0. I'll leave this bug open until the regression test gets in.
I’ve run into something similar when accidentally pressing return after entering «dbus monitor interface=». So, my command-line was «dbus monitor interface=», and this promptly brought down both the user and the system dbus (and oddly, all Gtk+ application windows vanished; out of curiosity: why does that happen?). Can you advise whether this is already covered here, or should I file this separately?
(In reply to freedesk.apriori from comment #15) > I’ve run into something similar when accidentally pressing return after > entering «dbus monitor interface=». So, my command-line was «dbus monitor > interface=», and this promptly brought down both the user and the system > dbus The system part of this report is unexpected. With dbus 1.10.0 on a non-Debian-derived distribution, I would expect this to bring down the session bus and your graphical login session; that's this bug, and does not need to be reported again. I'll release 1.10.2 soon to fix that. dbus on Debian and its derivatives is configured to try to tolerate assertion failures, mostly for historical reasons; that patch mitigates this. I'm tempted to apply the same thing upstream, either just for dbus-daemon or for all applications, but I'll open a separate bug to discuss that. I would not expect this to do anything to the system instance of dbus-daemon (typically running under uid "messagebus" or "dbus"). Please check your system log (syslog or systemd Journal) for log messages from the system instance of dbus-daemon when this happened. If the log confirms that the system instance of dbus-daemon crashed at the same time, please open a separate issue, with at least this information: * the distribution you are using * the version of dbus you had at the time of the crash, including the upstream version and the version of your distribution's packaging (for instance "Debian's dbus 1.10.0-3") * where I can find the patches and configuration that your distribution uses * system log messages from around the time of the crash > and oddly, all Gtk+ application windows vanished Applications that use the D-Bus session or system bus are expected to exit when that bus goes away, and many Gtk applications use D-Bus (via GApplication or GtkApplication, or directly), so this is not unexpected.
(In reply to Simon McVittie from comment #16) > dbus on Debian and its derivatives is configured to try to tolerate > assertion failures, mostly for historical reasons; that patch mitigates > this. I'm tempted to apply the same thing upstream, either just for > dbus-daemon or for all applications, but I'll open a separate bug to discuss > that. Bug #92546
Created attachment 119026 [details] [review] Keep cmake build system in sync with autotools (add test-monitor)
Comment on attachment 119026 [details] [review] Keep cmake build system in sync with autotools (add test-monitor) Review of attachment 119026 [details] [review]: ----------------------------------------------------------------- Fine, please apply
Created attachment 119027 [details] [review] Add a regression test for invalid BecomeMonitor calls --- This isn't expected to work on Windows until Bug #92538 is fixed.
Comment on attachment 118710 [details] [review] Add a regression test for invalid BecomeMonitor calls superseded
(In reply to Simon McVittie from comment #20) > Created attachment 119027 [details] [review] [review] > Add a regression test for invalid BecomeMonitor calls > > --- > > This isn't expected to work on Windows until Bug #92538 is fixed. I appled all patches from this bug, tried to run test-monitor.exe and got xxx@yyyy:~/src/dbus-1-cmake-cross-x86-build> DBUS_VERBOSE=1 bin/dbus-daemon.exe --session --address=tcp:host=localhost --print-address .... Failed to start message bus: Failed to open "S:/xxx/src/dbus-1-cmake-cross-x86-build/share\dbus-1\session.conf": Pfad nicht gefunden. The reason is that caused by commit 36d864e4697bc6e1eeffb32092bd78b108362ab5 session.conf and friends need to be created in the <build-root>/share/dbus-1/ instead of <build-root>/bus.
(In reply to Ralf Habacker from comment #22) > (In reply to Simon McVittie from comment #20) > > Created attachment 119027 [details] [review] [review] [review] > > Add a regression test for invalid BecomeMonitor calls > > > > --- > > > > This isn't expected to work on Windows until Bug #92538 is fixed. > > I appled all patches from this bug, tried to run test-monitor.exe and got > > xxx@yyyy:~/src/dbus-1-cmake-cross-x86-build> DBUS_VERBOSE=1 > bin/dbus-daemon.exe --session --address=tcp:host=localhost --print-address > .... > Failed to start message bus: Failed to open > "S:/xxx/src/dbus-1-cmake-cross-x86-build/share\dbus-1\session.conf": Pfad > nicht gefunden. > > The reason is that caused by commit 36d864e4697bc6e1eeffb32092bd78b108362ab5 > session.conf and friends need to be created in the > <build-root>/share/dbus-1/ instead of <build-root>/bus. From the cmake build root I'm able to run test-monitor with the following command line: DBUS_TEST_DAEMON=$PWD/bin/dbus-daemon.exe DBUS_TEST_DATA=z:$PWD/test/data bin/test-monitor.exe /monitor/invalid: OK /monitor/become: OK /monitor/broadcast: OK /monitor/forbidden-broadcast: OK /monitor/unicast-signal: OK /monitor/forbidden: OK /monitor/method-call: OK /monitor/forbidden-method: OK /monitor/dbus-daemon: OK /monitor/selective: OK /monitor/wildcard: OK /monitor/no-rule: OK /monitor/eavesdrop: OK /monitor/no-eavesdrop: OK I still got one message indicating a potential problem: /monitor/become: Connection :1.0 (pid=86 comm=""sid="S-1-5-21-0-0-0-1000" ) became a monitor. Monitoring connection :1.0 (pid=86 comm=""sid="S-1-5-21-0-0-0-1000" ) is not allowed to send messages; closing it. Please fix the monitor to not do that. (message type="method_call" interface="org.freedesktop.DBus" member="AddMatch" error name="(unset)" destination="org.freedesktop.DBus") Monitoring connection :1.0 closed.
(In reply to Ralf Habacker from comment #23) > /monitor/become: Connection :1.0 (pid=86 comm=""sid="S-1-5-21-0-0-0-1000" ) > became a monitor. > Monitoring connection :1.0 (pid=86 comm=""sid="S-1-5-21-0-0-0-1000" ) is not > allowed to send messages; closing it. Please fix the monitor to not do that. > (message type="method_call" interface="org.freedesktop.DBus" > member="AddMatch" error name="(unset)" destination="org.freedesktop.DBus") > Monitoring connection :1.0 closed. That isn't a problem. One of the pieces of functionality that is exercised by /monitor/become is "after a connection becomes a monitor, it is not allowed to send any more messages" - those messages are a side-effect of the dbus-daemon under test responding to that.
(In reply to Ralf Habacker from comment #23) > From the cmake build root I'm able to run test-monitor with the following > command line: > > DBUS_TEST_DAEMON=$PWD/bin/dbus-daemon.exe DBUS_TEST_DATA=z:$PWD/test/data > bin/test-monitor.exe That's correct. Just running the test executable in the built tree with no special arguments is not necessarily expected to work: the tests are primarily designed to run under "make check" (which sets environment variables via TESTS_ENVIRONMENT in the Makefile.am), or after installation.
(In reply to Simon McVittie from comment #20) > Created attachment 119027 [details] [review] > Add a regression test for invalid BecomeMonitor calls I've applied this unreviewed, because it doesn't affect the non-test parts of D-Bus, nobody has said no, and test > no test. Fixed in git for 1.10.2, 1.11.0.
Created attachment 119208 [details] attachment-24989-0.html Am 26.10.2015 um 13:51 schrieb bugzilla-daemon@freedesktop.org: > Simon McVittie <mailto:simon.mcvittie@collabora.co.uk> changed bug > 92298 <https://bugs.freedesktop.org/show_bug.cgi?id=92298> > What Removed Added > Status ASSIGNED RESOLVED > Resolution --- FIXED > > *Comment # 26 <https://bugs.freedesktop.org/show_bug.cgi?id=92298#c26> > on bug 92298 <https://bugs.freedesktop.org/show_bug.cgi?id=92298> from > Simon McVittie <mailto:simon.mcvittie@collabora.co.uk> * > (In reply to Simon McVittie from comment #20 <show_bug.cgi?id=92298#c20>) > > Created attachment 119027 [details] [review] <attachment.cgi?id=119027> [details] > <attachment.cgi?id=119027&action=edit> [review] > <page.cgi?id=splinter.html&bug=92298&attachment=119027> > Add a > regression test for invalid BecomeMonitor calls > > I've applied this unreviewed, because it doesn't affect the non-test parts of > D-Bus, nobody has said no, and test > no test. Unfortunally you applied the outdated patch, which results into compile errors. The correct patch is https://bugs.freedesktop.org/attachment.cgi?id=119027 Regards Ralf
(In reply to Simon McVittie from comment #20) > Created attachment 119027 [details] [review] [review] > Add a regression test for invalid BecomeMonitor calls > > --- > > This isn't expected to work on Windows until Bug #92538 is fixed. The patches in Bug #92538 are already committed ?
(In reply to Ralf Habacker from comment #27) > Created attachment 119208 [details] > attachment-24989-0.html > > Am 26.10.2015 um 13:51 schrieb bugzilla-daemon@freedesktop.org: > > Simon McVittie <mailto:simon.mcvittie@collabora.co.uk> changed bug > > 92298 <https://bugs.freedesktop.org/show_bug.cgi?id=92298> > > What Removed Added > > Status ASSIGNED RESOLVED > > Resolution --- FIXED > > > > *Comment # 26 <https://bugs.freedesktop.org/show_bug.cgi?id=92298#c26> > > on bug 92298 <https://bugs.freedesktop.org/show_bug.cgi?id=92298> from > > Simon McVittie <mailto:simon.mcvittie@collabora.co.uk> * > > (In reply to Simon McVittie from comment #20 <show_bug.cgi?id=92298#c20>) > > > Created attachment 119027 [details] [review] [review] <attachment.cgi?id=119027> [details] > > <attachment.cgi?id=119027&action=edit> [review] > > <page.cgi?id=splinter.html&bug=92298&attachment=119027> > Add a > > regression test for invalid BecomeMonitor calls > > > > I've applied this unreviewed, because it doesn't affect the non-test parts of > > D-Bus, nobody has said no, and test > no test. > Unfortunally you applied the outdated patch, which results into compile > errors. The correct patch is > https://bugs.freedesktop.org/attachment.cgi?id=119027 Forget this comment. I was misleaded because the patch commit id in the git repo (dc8bb270996a79be8f83c0c1dab286884b526aa) is not the same of this patch (is91c41d0ed16b2fff51170426a42f3c326eadd67a).
(In reply to Ralf Habacker from comment #28) > The patches in Bug #92538 are already committed ? Not all of them, but enough that test-monitor should be able to pass in 1.10.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.