From: Kyle Lippincott <spectral@google.com>
Date: Thu, 08 Feb 2024 19:52:35 GMT
Subject: Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range
Message-ID: <CAO_smVhsKHu0QrvpFbofd7y-Exhnk7=JUzffECNZQx=MWzmnsw@mail.gmail.com>
In-Reply-To: <xmqqil2yn3ey.fsf@gitster.g>

On Thu, Feb 8, 2024 at 11:48 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> René Scharfe <l.s.r@web.de> 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".
>

I just rediscovered that. I think this probably should be something
that caller (check_nonce) implements, then. Having a _mem function
implies to me (though I'm very new to this codebase) that it supports
embedded NULs, but that's not what's happening here.