Re: [PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 4, 2018, 03:44 UTC
Message-ID: <xmqqmuwb5i7k.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <xmqq8t7v6zpd.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 21 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  sequencer.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index b98690ecd41..aba03e9429a 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -175,6 +175,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
>>  			warning(_("invalid commit message cleanup mode '%s'"),
>>  				  s);
>>  
>> +		free(s);
>>  		return status;
>>  	}
>
> Shouldn't 's' now lose 'const'?  After all, git_config_string()
> gives you an allocated memory so...

Yikes. Should git_config_string() and git_config_pathname() take "char **dst" instead of "const char **" as their out-location parameter? They both assign a pointer to an allocated piece of memory for the caller to own or dispose of, but because of const-ness of the pointee their first parameter has, a caller like this one must declare "const char *s" yet is forced to call free() not to leak the value when it is done.