Re: [PATCH v2 1/2] blame: harden ignore-revs parser and tag peeling

From: Ravi Mistry <rmistry@google.com>
Date: Oct 10, 2026, 19:25 UTC
Message-ID: <20261010192515.212460-1-rmistry@google.com>
In-Reply-To: <xmqqjynrwjg0.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 14 quoted lines

> Maybe I am slow, but I do not immediately see why ignoring
> everything after the first NUL is a problem.  A call to
> strbuf_trim() is ineffective at trimming whitespace that appears
> immediately before such a NUL.  For example, while
>
>     cf9bdb1...f6092d # comment LF
>
> would feed the leading 'cf9bdb1...f6092d' part (after stripping
> whitespace before '#') to parse_oid_hex_algop(), this
>
>     cf9bdb1...f6092d NUL comment LF
>
> would keep the whitespace after '92d' and cause the parsing to
> fail.  I do not see any security implications here.

Show 9 quoted lines

> > +		if (memchr(sb.buf, '\0', sb.len))
> > +			die("invalid object name: %s", sb.buf);
>
> A file with such an entry is rejected and the entire operation is
> aborted as suspected attack attempt, which feels like striking the
> balance between usability and security at a wrong place.
>
> But a line with broken object name already is rejected with "die()"
> with the existing code, so it may be OK.

Agreed, there is no security implication there. Rejecting "<oid>\0garbage" (when there is no space before the NUL) was only for consistency with how other non-comment trailing garbage on a line is rejected with die(). If we end up rerolling the series, I am happy to drop the NUL check if you prefer.

Thanks!