Bug 65959

Summary: DBusString: _dbus_string_free() may crash if the previous _dbus_string_init() failed
Product: dbus Reporter: Chengwei Yang <chengwei.yang.cn>
Component: coreAssignee: Havoc Pennington <hp>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: 1.4.x   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: [PATCH] DBusString: fix may if try to free a uninitialized str
[PATCH] DBusString: fix may crash if try to free an uninitialized str

Description Chengwei Yang 2013-06-20 09:19:15 UTC
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.

...
      free (memory);
...
Comment 1 Chengwei Yang 2013-06-20 09:20:57 UTC
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;
      }
Comment 2 Chengwei Yang 2013-06-20 09:27:58 UTC
Created attachment 81100 [details] [review]
[PATCH] DBusString: fix may if try to free a uninitialized str
Comment 3 Chengwei Yang 2013-06-20 09:32:44 UTC
Created attachment 81101 [details] [review]
[PATCH] DBusString: fix may crash if try to free an uninitialized  str
Comment 4 Simon McVittie 2013-06-20 09:45:06 UTC
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.
Comment 5 Simon McVittie 2013-06-20 12:25:21 UTC
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.
Comment 6 Simon McVittie 2017-11-06 18:53:57 UTC
(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.
Comment 7 Simon McVittie 2017-11-06 18:54:43 UTC
(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.

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.