Re: PATCH: prevent a double free() with a malformed LDAP URL
Date: Thu, 05 Sep 2013 18:03:50 -0400
Daniel Stenberg wrote:
>> First, in the case of a well-formed URL, ludp->lud_attrs_dup is not
>> initialized prior calling
>> ludp->lud_attrs_dup[i] = curl_easy_unescape(data, ludp->lud_attrs[i],
>> 0, NULL);
>> inside unescape_elements().
> The struct is allocated with calloc() in _ldap_url_parse(), is it not?
Yes. When the struct is allocated using calloc(), that allocates space
for a pointer and sets the pointer to 0.
Nothing allocates memory for the array, however, so the pointer
lud_attrs_dup is still 0x00 when the code above uses the  operator.
That causes an attempt to dereference 0. I confirmed that this is
happening and causes a crash.
>> Second, in the case of a malformed URL, this loop:
>> + for(i = 0; ludp->lud_attrs_dup[i]; i++)
>> + free(ludp->lud_attrs_dup[i]);
>> runs in _ldap_free_urldesc() regardless of whether ludp->lud_attrs_dup
>> has been initialized.
> ... thus they should be NULL if not inited and free() works on NULL.
> Even if we in general avoid free on NULL in libcurl to better catch
> mistakes so I'd be fine with adding a check.
It's not freeing NULL that causes a crash. It's dereferencing NULL using
the  operator that causes a crash.
I think we just need to allocate the array after we count the elements
in ludp->lud_attrs, check to make sure the array's been allocated before
using the  operator to free its elements, and make sure inside
_ldap_free_urldesc() to free ludp->lud_attrs_dup if it's been allocated.
I've attached a modified version of your patch to show what I mean.
That's been tested on win32, and confirmed expected behavior for both
malformed and well-formed ldap urls.
(I just noticed that my editor on Windows messed up the indentation, and
I flubbed the subject line when using git format-patch, but I don't have
time to address that right at this moment. If this otherwise looks good,
I can fix that and resend if you want.)
- text/plain attachment: stored