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

[Junio C Hamano <gitster@pobox.com>]

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

>      ++	 * 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.

OK.  Because the caller didn't die with SIGBUS owhen they scanned
the memory to figure out len, we know scanning between msg[0] and
msg[len] is safe.

>      ++	 * Make this function safer by checking if the input is NUL-terminated
>      ++	 * or has a NL between line[0] and msg[len].

This comment however feels wrong.  The function is _safe_ if the
input is NUL-terminated already.  And it is very expected that the
msg[] has LF between msg[0] and msg[len]---after all that is why we
use strchrnul() to scan it.

Perhaps replace it with something like "But nevertheless it would be
better to make sure the function does not look at msg beyond len
that was provided by the caller", perhaps?

The most problematic case happens when msg points into somewhere
inside a page and &msg[len] is still inside that page, but (1) there
is no LF nor NUL in the page after &msg[0], and (2) the page after
the page &msg[0] and &msg[len] are in is not mapped.  Letting
strchrnul() scan msg[] beyond len will lead us to attempt to read
the first byte of the next page and killed with SIGBUS (I recall
that I had to debug a mysterious breakage in strlen or strchr on
SunOS, which tried to grab 8-byte at a time and crossed page
boundary to get an unnecessary SIGBUS).