Created attachment 124680 [details] [review] Cripple W32 socket credentials reader to NOT set the process ID How to reproduce: * Apply the patch attached * Run test-debug-daemon Expected result: * process_id test passes Actual result: ERROR:../../repo/test/dbus-daemon.c:548:test_processid: assertion failed (error.name == DBUS_ERROR_UNIX_PROCESS_ID_UNKNOWN): ("org.freedesktop.DBus.Error.InvalidArgs" == "org.freedesktop.DBus.Error.UnixProcessIdUnknown") What happens: _dbus_message_iter_get_args_valist() does not pass the "msg_type == spec_type" check and fails with DBUS_ERROR_INVALID_ARGS when processing DBUS_ERROR_UNIX_PROCESS_ID_UNKNOWN received from the daemon. This came up while i was testing my patch for bug 96577, which creates a more-preferable-than-EXTERNAL auth mechanism, which does not provide a PID. The patch attached here simulates that situation by crippling W32 implementation of socket credentials reading to *also* not provide a PID.
Created attachment 124681 [details] [review] Use dbus_set_error_from_message() to check for an error This patch did fix the test for me.
Comment on attachment 124681 [details] [review] Use dbus_set_error_from_message() to check for an error Review of attachment 124681 [details] [review]: ----------------------------------------------------------------- Looks basically right. Would you mind having a look for anywhere else that has the same bug? ::: test/dbus-daemon.c @@ +556,4 @@ > } > else > { > + g_assert_not_reached (); I'd prefer something like g_error ("Unexpected error: %s: %s", error.name, error.message); so we get better diagnostics.
(In reply to Simon McVittie from comment #2) > Comment on attachment 124681 [details] [review] [review] > Use dbus_set_error_from_message() to check for an error > > Review of attachment 124681 [details] [review] [review]: > ----------------------------------------------------------------- > > Looks basically right. Would you mind having a look for anywhere else that > has the same bug? I would mind.
Created attachment 124682 [details] [review] Use dbus_set_error_from_message() to check for an error v2: * Use g_error() for better diagnostic message on unexpected error
Comment on attachment 124682 [details] [review] Use dbus_set_error_from_message() to check for an error Review of attachment 124682 [details] [review]: ----------------------------------------------------------------- Looks good, I'll merge it soon.
(In reply to LRN from comment #3) > I would mind. No problem, I'll try to get to this myself eventually.
Fixed in git for 1.10.10, 1.11.4. Thanks! Leaving this bug open (but marking it as not containing a relevant patch any more) since I still need to check whether we've made the same mistake anywhere else.
Created attachment 125008 [details] [review] update-activation-environment: produce better diagnostics on error If dbus-daemon or systemd replied to our method call with an error, we would report it as "invalid arguments" instead of the true error name and message. Same root cause as <https://bugs.freedesktop.org/show_bug.cgi?id=96653>. --- Targeting 1.10 for this one since it's a user-facing diagnostic, unless reviewers feel that this change should only go to master.
Created attachment 125009 [details] [review] various tests: produce better diagnostics on error Same root cause as <https://bugs.freedesktop.org/show_bug.cgi?id=96653>: we didn't check whether the message was in fact an error reply. --- Intended for 1.11 only, since this is only a test issue and doesn't cause any known test failures.
(In reply to Simon McVittie from comment #8) > update-activation-environment: produce better diagnostics on error ... > Targeting 1.10 for this one since it's a user-facing diagnostic, unless > reviewers feel that this change should only go to master. It's been a month, and the change is fairly trivial, so I applied it unreviewed.
(In reply to Simon McVittie from comment #9) > various tests: produce better diagnostics on error Again, it's been a month, so I applied this unreviewed. Fixed in git for 1.10.10, 1.11.4.
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.