[PATCH] (backport) fix usage of deprecated D-Bus functions
Dan Williams
dcbw
Sat Mar 8 13:12:40 PST 2008
On Fri, 2008-03-07 at 16:49 +0200, Jouni Malinen wrote:
> On Tue, Mar 04, 2008 at 09:58:40AM -0500, Dan Williams wrote:
>
> > The purpose of the patch is to get rid of
> > dbus_message_iter_get_array_len(), which is deprecated because it
> > doesn't work everywhere or the way we expect it. Therefore, instead of
> > pre-sizing the arrays, we start with a reasonable size and grow the
> > arrays when needed while demarshalling the D-Bus message.
>
> Thanks! This looks otherwise fine, but the use of realloc looks a bit
> suspect. I did not go through all the possible cleanup paths, but it
> does not look like the old buffer would be freed in case of an
> allocation error (i.e., a corner case memory leak). Did I miss something
> or should this be fixed to make sure that the old buffer gets freed?
It should get cleared in wpa_dbus_dict_get_entry() if an error occurs
(like when the realloc would fail). Callers of
wpa_dbus_dict_get_entry() can expect that memory is correctly freed when
an error occurs.
> > + entry->bytearray_value = buffer;
>
> > + buffer = realloc(buffer, BYTE_ARRAY_ITEM_SIZE * (count + BYTE_ARRAY_CHUNK_SIZE));
> > + if (buffer == NULL) {
> > + perror("_wpa_dbus_dict_entry_get_byte_array["
> > + "dbus] out of memory trying to "
> > + "retrieve the string array");
> > + goto done;
> > + }
>
> Where will the old buffer (buffer prior to that realloc call) be freed?
> entry->bytearray_value has the buffer here, but it was not immediately
> obvious whether that is freed somewhere.
wpa_dbus_dict_get_entry() should clean the dict entry up on error.
> > - char **tmp;
> > - tmp = realloc(buffer, sizeof(char *) * (count + 8));
> > - if (tmp == NULL) {
> > + if ((count % STR_ARRAY_CHUNK_SIZE) == 0 && count != 0) {
> > + buffer = realloc(buffer, STR_ARRAY_ITEM_SIZE * (count + STR_ARRAY_CHUNK_SIZE));
> > + if (buffer == NULL) {
> > perror("_wpa_dbus_dict_entry_get_string_array["
> > "dbus] out of memory trying to "
> > "retrieve the string array");
> > - free(buffer);
> > - buffer = NULL;
> > goto done;
>
> Same here.. The old version was explicitly taking care of the error case
> and freeing the old buffer on realloc failure. The new version leaves a
> pointer to the old buffer into entry->strarray_value, but it was not
> clear whether that gets freed on error path.
>
> Are both of these freed in wpa_dbus_dict_entry_clear()? Is that function
> guaranteed to be called for all error cases?
Yes, both the string and byte array values are freed in
wpa_dbus_dict_entry_clear(), which should get called from
wpa_dbus_dict_get_entry() on any read error.
Thanks,
Dan
More information about the Hostap
mailing list