[PATCH] (backport) fix usage of deprecated D-Bus functions

[Dan Williams]

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