Bug 19367

Summary: time_t is assumed to be always the same size of gunit in text-mixin
Product: Telepathy Reporter: Sunil Mohan Adapa <sunil>
Component: tp-glibAssignee: Sunil Mohan Adapa <sunil>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Alternative patch

Description Sunil Mohan Adapa 2009-01-02 02:50:45 UTC
time_t is used for timestamp member in the pending message structure. According to the spec, this should always be a 32bit unsigned number. On Windows, since Visual Studio 2005 time_t is 64 bit.
Comment 1 Simon McVittie 2009-01-07 06:25:05 UTC
We should confirm this by looking at the code, but I believe our assumption is actually that time_t is *at least* 32 bits, which should be fine - it's truncated (where necessary) when turning the C value into the D-Bus type 'u'. We never do marshalling or pointer-arithmetic based on the assumption that time_t is exactly 32 bits.

(Similarly, dbus-glib represents the D-Bus type 'u' with guint by assuming that guint is at least 32 bits long.)
Comment 2 Sunil Mohan Adapa 2009-01-07 07:17:15 UTC
(In reply to comment #1)
> We never do marshalling or pointer-arithmetic based on the assumption that
> time_t is exactly 32 bits.

IIRC, the following code is what was causing the problem:

      g_value_init (&val, pending_type);
      g_value_take_boxed (&val,
          dbus_g_type_specialized_construct (pending_type));
      dbus_g_type_struct_set (&val,
          0, msg->id,
          1, msg->timestamp,
          2, msg->sender,
          3, msg->type,
          4, msg->flags,
          5, msg->text,
          G_MAXUINT);

The pending type has been registered as taking uuuuus and when the dbus_g_type_struct_set call is made, we are sending 8 bytes where 4 bytes are expected. The remaining 4 bytes will be parsed as next argument in the stack.
Comment 3 Simon McVittie 2009-01-07 09:24:59 UTC
Aha! OK, the bug is actually that we assume time_t to be the same size as guint (which may or may not be 32 bits - dbus-glib uses guint for type 'u' even if guint is actually larger than that).

Suggested patch:

       dbus_g_type_struct_set (&val,
           0, msg->id,
-          1, msg->timestamp,
+          1, (guint) msg->timestamp,
           2, msg->sender,
           3, msg->type,
           4, msg->flags,
           5, msg->text,
           G_MAXUINT);
Comment 4 Sunil Mohan Adapa 2009-01-08 01:50:38 UTC
Created attachment 21797 [details] [review]
Alternative patch

(In reply to comment #3)
> Aha! OK, the bug is actually that we assume time_t to be the same size as guint
> (which may or may not be 32 bits - dbus-glib uses guint for type 'u' even if
> guint is actually larger than that).

I understand now. Only dbus insists that 'u' be 32bits and dbus-glib will take guint for 'u' which could of any length.

> 
> Suggested patch:
[...]
> -          1, msg->timestamp,
> +          1, (guint) msg->timestamp,

It seems to me that s/time_t/guint/ is nicer. Why declare a different type than what spec defines and keep typecasting it every time? Assuming we don't want to touch the API, how about the patch attached?
Comment 5 Simon McVittie 2009-01-08 04:09:10 UTC
Using guint for 'u' is not a bug, even on ILP64 - it is part of dbus-glib's API/ABI that G_TYPE_UINT represents 'u'. It will take sizeof(guint) bytes off the stack, and marshal them into 32 bits in the message, truncating if necessary.

The first band of your patch (changing the type of priv->timestamp to be a guint) is indeed a better solution, which avoids altering telepathy-glib's API. Fixed in git, and we'll hopefully have a release later today.

The cast in the second band of your patch is actually unnecessary - the compiler knows what it needs to do - so I've left it out. The reason the current code is buggy is that dbus_g_type_struct_set is a varargs method, so the compiler has to guess at the intended types of the arguments; it will push sizeof(time_t) bytes onto the stack, but dbus_g_type_struct_set will pop sizeof(guint) bytes, leading to breakage if those sizes differ.
Comment 6 Sunil Mohan Adapa 2009-01-08 04:50:23 UTC
(In reply to comment #5)
> Using guint for 'u' is not a bug, even on ILP64 - it is part of dbus-glib's
> API/ABI that G_TYPE_UINT represents 'u'. It will take sizeof(guint) bytes off
> the stack, and marshal them into 32 bits in the message, truncating if
> necessary.

After your previous comment, I guessed this is what it must be doing.

> 
> The first band of your patch (changing the type of priv->timestamp to be a
> guint) is indeed a better solution, which avoids altering telepathy-glib's API.
> Fixed in git, and we'll hopefully have a release later today.

Thanks.

> 
> The cast in the second band of your patch is actually unnecessary - the
> compiler knows what it needs to do - so I've left it out.

OK. I merely added it for explicit clarity. 

> The reason the
> current code is buggy is that dbus_g_type_struct_set is a varargs method, so
> the compiler has to guess at the intended types of the arguments; it will push
> sizeof(time_t) bytes onto the stack, but dbus_g_type_struct_set will pop
> sizeof(guint) bytes, leading to breakage if those sizes differ.
> 

Yes, this is exactly what was happening. It was yielding some funny results with some members of the structure getting written twice and some members getting ignored etc.

I guess we can close the bug. Thanks again, Simon.

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.