Re: [PATCH 3/2] commit: remove find_header_mem()

From: Jeff King <peff@peff.net>
Date: Jun 19, 2024, 17:31 UTC
Message-ID: <20240619173106.GA1519400@coredump.intra.peff.net>
In-Reply-To: <0d85712c-5beb-4a64-a7f4-797782c26694@web.de>

On Wed, Jun 19, 2024 at 07:13:19PM +0200, René Scharfe wrote:

Show 6 quoted lines

> cfc5cf428b (receive-pack.c: consolidate find header logic, 2022-01-06)
> introduced find_header_mem() and turned find_commit_header() into a thin
> wrapper.  Since then, the latter has become the last remaining caller of
> the former.  Remove it to restore find_commit_header() to the state
> before cfc5cf428b, get rid of a strlen(3) call and resolve a NEEDSWORK
> note in the process.

That of course made me wonder what happened to the other caller(s) of find_header_mem(). The answer is that it went away in your 020456cb74 (receive-pack: use find_commit_header() in check_nonce(), 2024-02-09)

Show 6 quoted lines

> -const char *find_header_mem(const char *msg, size_t len,
> -			const char *key, size_t *out_len)
> +const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
>  {
>  	int key_len = strlen(key);
>  	const char *line = msg;

Not new in your patch, but assigning strlen() to int tingled my spider-sense. It's OK, though, because "key" is always a small string literal.

Show 10 quoted lines

> -	/*
> -	 * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
> -	 * given by len. However, current callers are safe because they compute
> -	 * len by scanning a NUL-terminated block of memory starting at msg.
> -	 * Nonetheless, it would be better to ensure the function does not look
> -	 * at msg beyond the len provided by the caller.
> -	 */
> -	while (line && line < msg + len) {
> +	while (line) {
>  		const char *eol = strchrnul(line, '\n');

OK, we no longer know the length of the message, but we don't need to because it's NUL terminated, and strchrnul() will find the correct eol. The length check might have saved us if we accidentally pushed "line" past the NUL terminator, but it looks like we take care not to do so in the loop body:

	line = *eol ? eol + 1 : NULL;

So the conversion looks good to me.

-Peff