Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 26, 2012, 16:13 UTC
Message-ID: <xmqq62cma2uo.fsf@junio.mtv.corp.google.com>
In-Reply-To: <87d36uxzfw.fsf@rho.meyering.net>

Jim Meyering <jim@meyering.net> writes:

Show 14 quoted lines

> What do you think about replacing those two append-if-needed two-liners:
>
>     if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
>             strbuf_addch(&buffer2, '/');
>
> by something that readably encapsulates the idiom:
>
>     strbuf_append_if_absent (&buffer2, '/');
>
> (though the name isn't particularly apt, because you might
> take "absent" to mean "not anywhere in the string," so maybe
>   strbuf_append_if_not_already_at_end (ugly) or
>   strbuf_append_uniq
> )

I am not good at names, but strbuf_terminate_with(&buffer2, '/') perhaps?

Show 9 quoted lines

> There are several other uses that would benefit from such a transformation:
> To find the easy ones, I ran this:
>
>   git grep -B1 "strbuf_addch.*'"|grep -A1 '!='
>
> I've manually marked/separated the ones that don't apply.
>
> Note how only 2 of the 6 candidates ensure that length is positive
> before using ".len - 1":

Yikes, that is embarrasing ;-)

Show 19 quoted lines

>
> ------------------------------------
> builtin/branch.c-	if (!buf.len || buf.buf[buf.len-1] != '\n')
> builtin/branch.c:		strbuf_addch(&buf, '\n');
> --
> builtin/fmt-merge-msg.c-		if (out->buf[out->len - 1] != '\n')
> builtin/fmt-merge-msg.c:			strbuf_addch(out, '\n');
> --
> builtin/log.c-		if (filename.buf[filename.len - 1] != '/')
> builtin/log.c:			strbuf_addch(&filename, '/');
> --
> builtin/notes.c-	if (buf.buf[buf.len - 1] != '\n')
> builtin/notes.c:		strbuf_addch(&buf, '\n'); /* Make sure msg ends with newline */
> --
> refs.c-		if (real_pattern.buf[real_pattern.len - 1] != '/')
> refs.c:			strbuf_addch(&real_pattern, '/');
> --
> strbuf.h-	if (sb->len && sb->buf[sb->len - 1] != '\n')
> strbuf.h:		strbuf_addch(sb, '\n');