Re: [PATCH 11/15] find multi-byte comment chars in unterminated buffers

From: Jeff King <peff@peff.net>
Date: Mar 12, 2024, 08:05 UTC
Message-ID: <20240312080525.GB47852@coredump.intra.peff.net>
In-Reply-To: <3f823e48-572c-4e19-ab76-e6d7cab9461f@web.de>

On Thu, Mar 07, 2024 at 08:42:22PM +0100, René Scharfe wrote:

Show 13 quoted lines

> > Arguably starts_with() and this new function should both be inlined,
> > like we do for skip_prefix(), but I think that's out of scope for this
> > series.
> 
> Inlining would allow the compiler to unroll the loop for string
> constants.  I doubt it would do that for variables, as in the code
> below.
> 
> Inlining the strlen()+memcmp() version above might allow the compiler
> to push the strlen() call out of a loop.
> 
> Would any of that improve performance noticeably?  For the call sites
> below I doubt it.  But it would probably increase the object text size.

Good point. With non-constant prefixes in these cases, it probably wouldn't buy much. There are a lot of other cases with actual string constants. A compiler in theory could turn starts_with(str, "foo") into a few instructions. But it's not even clear that it's in very many hot paths. It would definitely be something we'd have to measure.

Show 5 quoted lines

> > And it's possible I was simply too dumb to figure out xstrncmpz() here.
> > I'm waiting for René to show up and tell me how to do it. ;)
> 
> Nah, it's not a good fit, as it requires the two strings to have the
> same length.

Thanks for confirming I wasn't missing anything. :)

Show 12 quoted lines

> > @@ -2562,7 +2562,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
> >  	/* left-trim */
> >  	bol += strspn(bol, " \t");
> >
> > -	if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
> > +	if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
> 
> If the strspn() call is safe (which it is, as the caller expects the
> string to be NUL-terminated) then you could use starts_with() here and
> avoid the length calculation.  But that would also match
> comment_line_str values that contain LF, which the _mem version does not
> and that's better.

I try not to read too much into the use of string functions on what otherwise appears to be an unterminated buffer. While in Git it is quite often terminated at allocation time (coming from a strbuf, etc) I feel like I've fixed a number of out-of-bounds reads simply due to sloppy practices. And even if something is correct today, it is easy for it to change, since the assumption is made far away from allocation.

So I dunno. Like you said, fewer computations is fewer opportunity to mess things up. I don't like the idea of introducing a new hand-grenade that might blow up later, but maybe if it's right next to a strspn() call that's already a problem, it's not materially making anything worse.

Show 15 quoted lines

> > +int starts_with_mem(const char *str, size_t len, const char *prefix)
> > +{
> > +	const char *end = str + len;
> > +	for (; ; str++, prefix++) {
> > +		if (!*prefix)
> > +			return 1;
> > +		else if (str == end || *str != *prefix)
> > +			return 0;
> > +	}
> > +}
> 
> So this checks whether a length-limited string has a prefix given as a
> NUL-terminated string.  I'd have called it mem_starts_with() and have
> expected starts_with_mem() to check a NUL-terminated string for a
> length-limited prefix (think !strncmp(str, prefix, prefixlen)).

I was going for consistency with skip_prefix_mem() and strip_suffix_mem(). To be fair, I probably also named those ones, but I think it's pretty established. We've never needed the length-limited prefix variant yet, so I don't know that we're squatting on anything too valuable.

Show 12 quoted lines

> > @@ -882,7 +882,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
> >
> >  	/* The first paragraph is the title and cannot be trailers */
> >  	for (s = buf; s < buf + len; s = next_line(s)) {
> > -		if (s[0] == comment_line_char)
> > +		if (starts_with_mem(s, buf + len - s, comment_line_str))
> >  			continue;
> >  		if (is_blank_line(s))
> 
> Another case where starts_with() would be safe to use, as
> is_blank_line() expects (and gets) a NUL-terminated string, but it would
> allow matching comment_line_str values that contain LF.

Hmm. Yes, it is a NUL-terminated string always, but the caller has told us not to look past end_of_log_message(). I suspect that if there is no newline in comment_line_str() it's probably impossible to go past "len" (just because the end of the log surely ends with either a NUL or a newline). But it feels iffy to me. I dunno.

-Peff