Bug 96653 - test-dbus-daemon process_id fails due to wrong message format on unset PID
Summary: test-dbus-daemon process_id fails due to wrong message format on unset PID
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other Windows (All)
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-06-23 12:18 UTC by LRN
Modified: 2016-08-12 09:26 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Cripple W32 socket credentials reader to NOT set the process ID (836 bytes, patch)
2016-06-23 12:18 UTC, LRN
Details | Splinter Review
Use dbus_set_error_from_message() to check for an error (1.61 KB, patch)
2016-06-23 12:28 UTC, LRN
Details | Splinter Review
Use dbus_set_error_from_message() to check for an error (1.65 KB, patch)
2016-06-23 13:00 UTC, LRN
Details | Splinter Review
update-activation-environment: produce better diagnostics on error (1.61 KB, patch)
2016-07-11 10:32 UTC, Simon McVittie
Details | Splinter Review
various tests: produce better diagnostics on error (2.34 KB, patch)
2016-07-11 10:33 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description LRN 2016-06-23 12:18:27 UTC
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.
Comment 1 LRN 2016-06-23 12:28:39 UTC
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 2 Simon McVittie 2016-06-23 12:35:04 UTC
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.
Comment 3 LRN 2016-06-23 12:57:43 UTC
(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.
Comment 4 LRN 2016-06-23 13:00:49 UTC
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 5 Simon McVittie 2016-06-23 15:45:46 UTC
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.
Comment 6 Simon McVittie 2016-06-23 16:15:41 UTC
(In reply to LRN from comment #3)
> I would mind.

No problem, I'll try to get to this myself eventually.
Comment 7 Simon McVittie 2016-06-30 15:43:06 UTC
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.
Comment 8 Simon McVittie 2016-07-11 10:32:31 UTC
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.
Comment 9 Simon McVittie 2016-07-11 10:33:53 UTC
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.
Comment 10 Simon McVittie 2016-08-12 09:13:04 UTC
(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.
Comment 11 Simon McVittie 2016-08-12 09:26:23 UTC
(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.