René Scharfe <l.s.r@xxxxxx> writes:
> Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
>> ---
>> 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.
>> -
>> - 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.
