Re: [PATCH v3] receive-pack.c: consolidate find header logic

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 4, 2022, 01:56 UTC
Message-ID: <xmqq35m4gt5x.fsf@gitster.g>
In-Reply-To: <pull.1125.v3.git.git.1640931460758.gitgitgadget@gmail.com>

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +	size_t out_len;
> +	const char *val = find_header_mem(msg, len, key, &out_len);
> +
> +	if (val == NULL)

Style:

	if (!val)

Show 26 quoted lines

> +		return NULL;
> +
> +	if (next_line)
> +		*next_line = val + out_len + 1;
> +
> +	return xmemdupz(val, out_len);
>  }
>  
>  /*
> diff --git a/commit.c b/commit.c
> index a348f085b2b..8ac32a4d7b5 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1631,12 +1631,13 @@ struct commit_list **commit_list_append(struct commit *commit,
>  	return &new_commit->next;
>  }
>  
> -const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
> +const char *find_header_mem(const char *msg, size_t len,
> +			const char *key, size_t *out_len)
>  {
>  	int key_len = strlen(key);
>  	const char *line = msg;
>  
> +	while (line && line < msg + len) {
>  		const char *eol = strchrnul(line, '\n');

Between line[0] and msg[len], there may not be a LF nor NUL at all, and strchrnul() will scan beyond the range we were given (which is msg[0]..msg[len]).

But that is something we share with the find_header() if I am not mistaken, so I am OK to leave the code as posted and leave it outside the scope of this series to clean it up to make it safer.

The reason why it is probably safe for the current set of callers (and presumably any reasonable new callers we may add later) is that they computed "len" by scanning the block of memory starting at (or before) "msg" before calling us, and we know that the block of memory is properly NUL-terminated. find_header() is called by check_nonce() and check_cert_push_options(), both of which tell us to scan in a strbuf, which is designed to be scannable for NUL safely by having an extra NUL at the end beyond the end.