Re: [RFC PATCH 01/15] remote.c: don't dereference NULL in freeing loop

From: Glen Choo <chooglen@google.com>
Date: Jun 3, 2022, 22:32 UTC
Message-ID: <kl6l35gle5u9.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <0c296c0e-bbd4-57a6-1cab-3b98c3bc8927@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 25 quoted lines

> Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  remote.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/remote.c b/remote.c
>> index 930fdc9c2f6..61240562df1 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -144,14 +144,10 @@ static void remote_clear(struct remote *remote)
>>  	free((char *)remote->name);
>>  	free((char *)remote->foreign_vcs);
>>
>> -	for (i = 0; i < remote->url_nr; i++) {
>> +	for (i = 0; i < remote->url_nr; i++)
>>  		free((char *)remote->url[i]);
>> -	}
>> -	FREE_AND_NULL(remote->pushurl);
>
> So you remove the redundant release of the pushurl array, but the url
> array, which should be freed here after its elements are released, still
> leaks.  The duplicate FREE_AND_NULL perhaps resulted from a copy&paste
> error.

This seems like the most likely explanation (even I don't recall how I ended up introducing this bug..). I probably meant to change pushurl -> url, but forgot somehow.

Show 11 quoted lines

>> -
>> -	for (i = 0; i < remote->pushurl_nr; i++) {
>> +	for (i = 0; i < remote->pushurl_nr; i++)
>>  		free((char *)remote->pushurl[i]);
>> -	}
>>  	FREE_AND_NULL(remote->pushurl);
>
> Why set pushurl to NULL after release?  This results in an invalid state
> unless pushurl_nr und pushurl_alloc are reset to zero.  Same goes for
> the url array above -- either a simple free(3) call suffices or url_nr
> and url_alloc need to be cleared as well.

True, thanks for catching that. We'd probably never notice it because we never reuse the clear-ed `struct remote` (since we only ever call this when we clean up `struct repository`), but it's better to do the 'right thing'.

I didn't consider that when I chose to use FREE_AND_NULL() instead of free(), and there wasn't any particular reason why I chose one over the other.