Bug 19367

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.)

(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.

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);

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?

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.

(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.