Re: [PATCH v2] diff --no-index: unleak paths[] elements

[Junio C Hamano <gitster@pobox.com>]

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

> Am 03.09.22 um 01:49 schrieb Junio C Hamano:
>> "git diff --no-index" codepath starts with the two elements in
>> argv[] and munges them into two paths to be compared, stored in a
>> separate path[] arrays.  The munging is implemented in a rather
>> haphazard way, sometimes overwriting old version with a new copy,
>> and sometimes a constant string assigned to path[], making it
>> impossible to release the resources properly:
>>
>>  * A single dash "-" from the command line is a special signal that
>>    the standard input is used for the side to be compared, and is
>>    internally replaced with a copy of string "-" at a known address.
>>
>>  * When run in a subdirectory, full paths to the two paths are
>>    allocated and placed in path[].
>>
>>  * After the above happens, when comparing a file with a directory,
>>    the directory side is replaced with the path to a file in the
>>    directory with the same name as the file.
>>
>> This was perfectly fine for just two strings that are pathnames used
>> during the lifetime of the program and cleaned up upon program exit,
>> but it gets in the way when leak sanitizer is in effect.  The third
>> step can be losing the full path that was allocated in the second
>> step, but it is not easy to tell if its input is an allocated piece
>> of memory to begin with.
>>
>> Loosen the earlier two steps a bit so that elements of the path[]
>> array that come to the directory/file comparison code are either the
>> singleton "-" or an allocated piece of memory.  Use that knowledge
>> in the third step to release an allocated piece of memory when it
>> replaces the path to a directory with the path to a file in that
>> directory, and also at the end to release the two elements of the
>> path[] array as needed.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>
>>  * The previous one allowed strbuf_release() to free replacement.buf
>>    which may be used in path[0] or path[1] potentially leading to
>>    double freeing.  The kosher way may be to use strbuf_detach() in
>>    fixup_paths(), but this is a simpler fix, it is getting late in
>>    the day, and I am getting sick of fighting the leak-checker, so...
>>
>>  diff-no-index.c | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>>
>> diff --git a/diff-no-index.c b/diff-no-index.c
>> index 9a8b09346b..77a126469b 100644
>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -208,6 +208,14 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi
>>  	strbuf_addstr(path, tail ? tail + 1 : file);
>>  }
>>
>> +static void free_allocated_path(const char *path)
>> +{
>> +	if (!path ||
>
> How can path be NULL?  And if it was, why shield free(3) from it?

See the comment under three-dashes of the first iteration.

>> +	/*
>> +	 * do not strbuf_release(&replacement), as it is in paths[]
>> +	 * when replacement was actually used.
>> +	 */
>> +	free_allocated_path(paths[0]);
>> +	free_allocated_path(paths[1]);
>>
>>  	/*
>>  	 * The return code for --no-index imitates diff(1):
>
> Perhaps avoid the need for that comment by moving that strbuf to where
> it's used and have it spend its full lifecycle there?  Something like:

Yup, that is what I said in the comment under three-dashes (with the
reason why I didn't bother).

Quite honestly I am sick of fighting the overzealous leak-checker so
I'd very much appreciate if somebody else pick this up and run with
it.

Thanks.