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

From: Junio C Hamano <[email-masked]>
Date: Fri, 03 Jun 2022 14:28:06 -0700

René Scharfe <[email-masked]> writes:

>> -	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.

We probably should give a huge warning next to FREE_AND_NULL() about
this.  It also is an effective way to hide an existing bug under the
rug.  diff_options.pathspec might be freed prematurely which may be
noticed by a use-after-free if left to use free(), but FREE_AND_NULL()
will mislead the use-after-free caller into thinkig that "ah there is
no pathspec to be used" and produce nonsense result without crashing.

Thanks.