On Thu, Feb 08, 2024 at 11:48:05AM -0800, Junio C Hamano wrote:
> René Scharfe <l.s.r@xxxxxx> writes:
>
> > But anyway: If NULs are of no concern and we currently end parsing when
> > we see one in all cases, why do we need a _mem function at all? The
> > original version of the function, find_commit_header(), should suffice.
> > check_nonce() could be run against the NUL-terminated sigcheck.payload
> > and check_cert_push_options() parses an entire strbuf, so there is no
> > risk of out-of-bounds access.
>
> If I recall correctly, the caller that does not pass strlen() as the
> payload length gives a length that is shorter than the buffer, i.e.
> "stop the parsing here, do not get confused into thinking the
> garbage after this point contains useful payload" was the reason why
> we have a separate "len".
Yes, check_nonce() passes in a length limited by the start of the actual
signature, as determined by parse_signed_buffer(). Though that generally
comes after a blank line, which would also stop find_header() from
parsing further.
But more interestingly: even though we pass a buf/len pair to
parse_signed_buffer(), it then calls get_format_by_sig() which takes
only a NUL-terminated string. So:
1. It is not possible for the buf/len pair we pass to check_nonce() to
contain a NUL. And thus there is no caller of find_header_mem()
that can contain an embedded NUL. So switching from strchrnul() to
just memchr() should be OK there.
2. That raises the question of whether parse_signed_buffer() has a
similar walk-too-far problem. ;) The answer is no, because we feed
it from a strbuf. But it's not a great pattern overall.
-Peff
