Re: [PATCH v3 1/3] dir: change the scope of function 'directory_exists_in_index()'

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 19, 2020, 00:47 UTC
Message-ID: <xmqqsg82tyeo.fsf@gitster.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.2012190104140.56@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 14 quoted lines

> Hi Shourya,
>
> On Tue, 15 Dec 2020, Shourya Shukla wrote:
>
>> Change the scope of the function 'directory_exists_in_index()' as well
>> as declare it in 'dir.h'.
>>
>> Since the return type of the function is the enumerator 'exist_status',
>> change its scope as well and declare it in 'dir.h'. While at it, rename
>> the members of the aforementioned enum so as to avoid any naming clashes
>> or confusions later on.
>
> This makes it sound as if only existing code was adjusted, in a minimal
> way, but no new code was introduced. But that's not true:

I noticed it last night, too---I suspect it was a mistake made while shuffling changes across steps with rebase -i.

Show 10 quoted lines

>> Helped-by: Christian Couder <christian.couder@gmail.com>
>> Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
>> Signed-off-by: Shourya Shukla <periperidip@gmail.com>
>> ---
>>  builtin/submodule--helper.c | 408 ++++++++++++++++++++++++++++++++++++
>>  dir.c                       |  30 ++-
>>  dir.h                       |   9 +
>>  3 files changed, 429 insertions(+), 18 deletions(-)
>
> Tons of new code there. And unfortunately...

Show 20 quoted lines

>> +static const char *parse_token(const char *cp, int *len)
>> +{
>> +	const char *p = cp, *start, *end;
>> +	char *str;
>> +
>> +	start = p;
>> +	while (*p != ' ')
>> +		p++;
>> +	end = p;
>> +	str = xstrndup(start, end - start);
>> +
>> +	while(*p == ' ')
>> +		p++;
>> +
>> +	return str;
>> +}
>
> This function is not careful enough to avoid buffer overruns. It even
> triggers a segmentation fault in our test suite:
> https://github.com/gitgitgadget/git/runs/1574891976?check_suite_focus=true#step:6:3152

I notice that len is not used at all ;-)

Show 53 quoted lines

> I need this to make it pass (only tested locally so far, but I trust you
> to take the baton from here):
>
> -- snipsnap --
> From c28c0cd3ac21d546394335957fbaa350ab287c3f Mon Sep 17 00:00:00 2001
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date: Sat, 19 Dec 2020 01:02:04 +0100
> Subject: [PATCH] fixup??? dir: change the scope of function
>  'directory_exists_in_index()'
>
> This fixes the segmentation fault reported in the linux-musl job of our
> CI builds. Valgrind has this to say about it:
>
> ==32354==
> ==32354== Process terminating with default action of signal 11 (SIGSEGV)
> ==32354==  Access not within mapped region at address 0x5C73000
> ==32354==    at 0x202F5A: parse_token (submodule--helper.c:2837)
> ==32354==    by 0x20319B: report_fetch_remotes (submodule--helper.c:2871)
> ==32354==    by 0x2033FD: add_submodule (submodule--helper.c:2898)
> ==32354==    by 0x204612: module_add (submodule--helper.c:3146)
> ==32354==    by 0x20478A: cmd_submodule__helper (submodule--helper.c:3202)
> ==32354==    by 0x12655E: run_builtin (git.c:458)
> ==32354==    by 0x1269B4: handle_builtin (git.c:712)
> ==32354==    by 0x126C79: run_argv (git.c:779)
> ==32354==    by 0x12715C: cmd_main (git.c:913)
> ==32354==    by 0x2149A2: main (common-main.c:52)
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/submodule--helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 4f1d892b9a9..29a6f80b937 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2834,12 +2834,12 @@ static const char *parse_token(const char *cp, int *len)
>  	char *str;
>
>  	start = p;
> -	while (*p != ' ')
> +	while (*p && *p != ' ')
>  		p++;
>  	end = p;
>  	str = xstrndup(start, end - start);
>
> -	while(*p == ' ')
> +	while(*p && *p == ' ')
>  		p++;
>
>  	return str;
> --
> 2.29.2.windows.1.1.g3464b98ce68