My bad Dirk - you're right with that one.
I'll take a look at this when I get home, and either apply your fix or
disentangle this in a hopefully more obvious way.
On Mon, Apr 21, 2014 at 1:53 PM, Dirk Engling <erdge...@erdgeist.org> wrote:
> On 21.04.14 19:01, Bob Beck wrote:
>
>> Not quite, because now you avoid the potential double free and instead leak
>> ret itself because of how ASN1_STRING_free works.. You need to
>> do this slightly differently.
>
> I disagree:
>
> err:
> if ((ret != NULL) && ((a == NULL) || (*a != ret)))
> ASN1_STRING_free(ret);
>
> clearly, ret is not NULL and in order to have a ASN1_STRING we need to
> free, the if ((a == NULL) || ((*a) == NULL)) on top must have triggered.
> So indeed ASN1_STRING_free is called with
>
> The ASN1_STRING_free function itself is rather straight forward:
>
> void
> ASN1_STRING_free(ASN1_STRING *a)
> {
> if (a == NULL)
> return;
> if (a->data && !(a->flags & ASN1_STRING_FLAG_NDEF))
> free(a->data);
> free(a);
> }
>
> IOW: it only does abort early and thus not free() ret, if ret was NULL,
> which is not the case once we get here. It will then free() ret->data if
> it is not NULL (the double free case I wanted to solve) and then goes on
> to free() ret.
>
> Maybe I have missed the magic where a NULL data field in ASN1_STRING
> makes ASN1_STRING_free not free the object, but I can't find that, at
> least not in my source tree.
>
> erdgeist
>
