Re: [RFC PATCH 01/15] remote.c: don't dereference NULL in freeing loop
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 3, 2022, 21:28 UTC
- Message-ID
- <xmqqilphcu8p.fsf@gitster.g>
- In-Reply-To
- <0c296c0e-bbd4-57a6-1cab-3b98c3bc8927@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 10 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.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.