Bug 65959

for e.g. At least I found the below code slice is not safe.

dbus/dbus-nonce.c: do_check_nonce()

  if (   !_dbus_string_init (&buffer)
      || !_dbus_string_init (&p) ) {
        dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
        _dbus_string_free (&p);
        _dbus_string_free (&buffer);
        return FALSE;
      }

Created attachment 81100 [details] [review]
[PATCH] DBusString: fix may if try to free a uninitialized str

Created attachment 81101 [details] [review]
[PATCH] DBusString: fix may crash if try to free an uninitialized  str

Comment on attachment 81101 [details] [review]
[PATCH] DBusString: fix may crash if try to free an uninitialized  str

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

::: dbus/dbus-string.c
@@ +251,5 @@
> +   * _dbus_string_init call
> +   * Bug: https://bugs.freedesktop.org/show_bug.cgi?id=65959
> +   */
> +  if (real->str == NULL)
> +    return;

I was about to ask: why would align_offset ever be nonzero for a string whose allocation had failed? But _dbus_string_init_preallocated says:

  /* It's very important not to touch anything
   * other than real->str if we're going to fail,
   * since we also use this function to reset
   * an existing string, e.g. in _dbus_string_steal_data()
   */

... which seems rather ridiculous, but welcome to libdbus. :-(

Patch looks good, I'll merge it soon.

Fixed in git for 1.6.14, 1.7.6.

This is a rather unlikely crash: on modern Unix systems (including Linux), malloc() basically never fails, because the kernel prefers to overcommit memory. dbus does claim to support malloc() failing, though.

(In reply to Chengwei Yang from comment #0)
> If the _dbus_string_init(arg) failed with OOM, say arg->str is NULL now, if
> then _dbus_string_free(arg) invoked to free the arg, it's not safe, see the
> below slice.
> 
> ...
>   dbus_free (real->str - real->align_offset);
> ...
> 
> and in dbus_free(void *memory)
> ...
>   if (memory) /* we guarantee it's safe to free (NULL) */
> 
> however, the above case will give memory an undefined invalid pointer, so
> free() may crash.

I started looking back at this because it's related to Bug #89104.

In the interests of correcting the historical record:

Whatever code had this calling pattern was wrong. Before _dbus_string_init(), @arg had undefined contents. After _dbus_string_init(), @arg *still* had undefined contents, because functions that fail . Calling _dbus_string_free() on a structure with undefined contents is programming error (undefined behaviour).

Bug #89104 aims to make this more sensible, by having something to which you can validly initialize a DBusString with no possibility of it failing, analogous to DBUS_ERROR_INIT.

(In reply to Chengwei Yang from comment #1)
> for e.g. At least I found the below code slice is not safe.
> 
> dbus/dbus-nonce.c: do_check_nonce()
> 
>   if (   !_dbus_string_init (&buffer)
>       || !_dbus_string_init (&p) ) {

This code is wrong. I've opened Bug #103597.

(In reply to Simon McVittie from comment #6)
> Whatever code had this calling pattern was wrong. Before
> _dbus_string_init(), @arg had undefined contents. After _dbus_string_init(),
> @arg *still* had undefined contents, because functions that fail .

This should have said: because functions that fail are not normally expected to alter their "out" arguments.