René Scharfe <l.s.r@xxxxxx> 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@xxxxxxxxx>
>> ---
>>
>> * 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.
