Re: [PATCH v2] commit.c: ensure find_header_mem() doesn't scan beyond given range

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 7, 2024, 17:23 UTC
Message-ID: <xmqqr0ho9oi9.fsf@gitster.g>
In-Reply-To: <e7b269ea-6a10-4f3c-ae97-a58eb7ccc6ef@web.de>

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

Show 26 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) {
>> -		const char *eol = strchrnul(line, '\n');
>> +		char *eol = (char *) line;
>> +		for (size_t i = 0; i < len && *eol && *eol != '\n'; i++) {
>> +			eol++;
>> +		}
>
> This uses the pointer eol only for reading, so you can keep it const.
>
> The loop starts counting from 0 to len for each line, which cannot be
> right.  find_header_mem("headers\nfoo bar", 9, "foo", &len) would still
> return "bar" instead of NULL.
>
> You could initialize i to the offset of line within msg instead (i.e.
> i = line - msg).  Or check eol < msg + len instead of i < len -- then
> you don't even need to introduce that separate counter.
>
> Style nit: We tend to omit curly braces if they contain only a single
> statement.

All true. As we already use an extra variable 'i' for counting, we can do without eol and reference line[i] instead, which would make the whole thing something like

	while (line && line < msg + len) {
		size_t i;
		for (i = 0;
                     i < len && line[i] && line[i] != '\n';
		     i++)
			;
		if (key_len < i &&
		    !strncmp(line, key, ken_len) &&
		    linhe[key_len] == ' ') {
			*out_len = i - key_len - 1;
			return line + key_len + 1;
		}
                line = line[i] ? line + i + 1 : NULL;
	}

which is not too bad, simply because the original already needed to know the length of the current line and due to lack of this "i" you introduced, it used "eol-line" instead. Now you have "i", the code may get even simpler by getting rid of "eol".