Bug 48580 - Broken DBUS_COOKIE_SHA1 client
Summary: Broken DBUS_COOKIE_SHA1 client
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard: review-
Keywords: patch
: 28564 (view as bug list)
Depends on:
Blocks: dbus-1.5
  Show dependency treegraph
 
Reported: 2012-04-11 20:05 UTC by David Zeuthen (not reading bugmail)
Modified: 2013-08-27 16:49 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch (3.67 KB, patch)
2012-04-11 20:10 UTC, David Zeuthen (not reading bugmail)
Details | Splinter Review
Updated patch (13.07 KB, patch)
2012-04-12 06:45 UTC, David Zeuthen (not reading bugmail)
Details | Splinter Review
Patch v3 (14.77 KB, patch)
2012-04-12 07:55 UTC, David Zeuthen (not reading bugmail)
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Zeuthen (not reading bugmail) 2012-04-11 20:05:17 UTC
When we moved to using monotonic time, support for DBUS_COOKIE_SHA1 was broken, in particular interoperability with non-libdbus-1 implementations such as GDBus.  The problem is that, if monotonic clocks are available, _dbus_get_current_time() won't return the number of seconds since the Epoch (as required by the D-Bus spec).

This bug was originally reported here

 https://bugzilla.gnome.org/show_bug.cgi?id=673943
Comment 1 David Zeuthen (not reading bugmail) 2012-04-11 20:10:54 UTC
Created attachment 59826 [details] [review]
Patch

This patch fixes the problem for me - I've also double-checked that it works with GDBus, see https://bugzilla.gnome.org/show_bug.cgi?id=673943 for more details.

Once a D-Bus release with this bug-fix is available, GDBus will include a test-case for DBUS_COOKIE_SHA1 interop with libdbus-1 (and require this version).
Comment 2 Simon McVittie 2012-04-12 01:04:37 UTC
Comment on attachment 59826 [details] [review]
Patch

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

We should fix this before 1.6, and possibly also in a 1.4.x release.

::: dbus/dbus-sysdeps-unix.c
@@ +2631,5 @@
> + * @param tv_sec return location for number of seconds
> + * @param tv_usec return location for number of microseconds (thousandths)
> + */
> +void
> +_dbus_get_current_time_real (long *tv_sec,

I'd prefer _dbus_get_real_time().

Even better would be if you renamed _dbus_get_current_time() to _dbus_get_monotonic_time() to make the difference clearer: this would also reassure me that someone had looked at each caller and confirmed "yes, we do want monotonic time there".

::: dbus/dbus-sysdeps-win.c
@@ +1904,5 @@
> +void
> +_dbus_get_current_time_real (long *tv_sec,
> +                             long *tv_usec)
> +{
> +  return _dbus_get_current_time (tv_sec, tv_usec);

"return x()", where x returns void, is non-portable. Remove the return keyword.

Assuming the Windows code only has wall-clock time and not monotonic time, I'd prefer this to be the other way round:

void
_dbus_get_monotonic_time (long *tv_sec, long *tv_usec)
{
  /* no implementation yet, fall back to wall-clock time */
  _dbus_get_real_time (tv_sec, tv_usec);
}

void
_dbus_get_real_time()
{
  [real implementation here]
}
Comment 3 David Zeuthen (not reading bugmail) 2012-04-12 06:45:57 UTC
Created attachment 59858 [details] [review]
Updated patch

Thanks for the review, here's an updated patch.
Comment 4 Simon McVittie 2012-04-12 07:42:16 UTC
Comment on attachment 59858 [details] [review]
Updated patch

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

::: dbus/dbus-sysdeps-unix.c
@@ +2625,5 @@
>  }
>  
>  /**
> + * Get current time, as in gettimeofday(). Never uses the monotonic
> + * clock.

I'd prefer this one to be called _dbus_get_real_time() (or epoch_time or wallclock_time or something), so we no longer have a _dbus_get_current_time() at all, to avoid the ambiguity of "which one does 'current' mean?".

::: dbus/dbus-sysdeps-win.c
@@ +1877,3 @@
>   *
>   * @param tv_sec return location for number of seconds
> + * @param tv_usec return location for number of microseconds (thousandths)

A microsecond is a millionth. (This is a pre-existing bug, now I look more closely: the Unix version was documented wrong already.)
Comment 5 David Zeuthen (not reading bugmail) 2012-04-12 07:55:04 UTC
Created attachment 59863 [details] [review]
Patch v3

Thanks again for the review. Updated patch attached.
Comment 6 Simon McVittie 2012-04-12 08:02:46 UTC
Comment on attachment 59863 [details] [review]
Patch v3

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

Yes please! Please merge to master, and to dbus-1.4 if you want it there.
Comment 7 David Zeuthen (not reading bugmail) 2012-04-12 08:19:36 UTC
Excellent, thanks for the quick review!

master (will be in dbus 1.5.13): http://cgit.freedesktop.org/dbus/dbus/commit/?id=8734e4a16ff220a7af0fd718ba50f92d23c496cf

dbus-1.4 (will be in dbus 1.4.21): http://cgit.freedesktop.org/dbus/dbus/commit/?h=dbus-1.4&id=870351439769a35344ead435f2490ed933dd8ff4
Comment 8 Simon McVittie 2013-08-27 16:49:24 UTC
*** Bug 28564 has been marked as a duplicate of 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.