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

[Jim Meyering <jim@xxxxxxxxxxxx>]

Junio C Hamano wrote:
> Jim Meyering <jim@xxxxxxxxxxxx> writes:
>
>> Marcus Karlsson wrote:
>> ...
>>> Are there any guarantees that len1 and len2 does not exceed PATH_MAX?
>>> Because if there aren't any then that function looks like it could need
>>> even more improvements.
>>
>> Hi Marcus,
>>
>> You're right to ask.
>> I've just confirmed that there is such a guarantee.
>
> In any case, I think this is an old part of the codebase that has not
> been updated to take advantage of newer API, partly because not many
> people cared, and partly because there wasn't any serious bug there,
> that can use some facelifting.  Wouldn't it make more sense to use
> strbuf here, perhaps like this (not even compile tested), on top of your
> patch?
>
>  diff-no-index.c |   40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 5cd3ff5..b44473e 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
>  }
>
>  static int queue_diff(struct diff_options *o,
> -		const char *name1, const char *name2)
> +		      const char *name1, const char *name2)
>  {
>  	int mode1 = 0, mode2 = 0;
>
> @@ -63,10 +63,11 @@ static int queue_diff(struct diff_options *o,
>  		return error("file/directory conflict: %s, %s", name1, name2);
>
>  	if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
> -		char buffer1[PATH_MAX], buffer2[PATH_MAX];
> +		struct strbuf buffer1 = STRBUF_INIT;
> +		struct strbuf buffer2 = STRBUF_INIT;
>  		struct string_list p1 = STRING_LIST_INIT_DUP;
>  		struct string_list p2 = STRING_LIST_INIT_DUP;
> -		int len1 = 0, len2 = 0, i1, i2, ret = 0;
> +		int i1, i2, ret = 0;
>
>  		if (name1 && read_directory(name1, &p1))
>  			return -1;
> @@ -76,19 +77,15 @@ static int queue_diff(struct diff_options *o,
>  		}
>
>  		if (name1) {
> -			len1 = strlen(name1);
> -			if (len1 > 0 && name1[len1 - 1] == '/')
> -				len1--;
> -			memcpy(buffer1, name1, len1);
> -			buffer1[len1++] = '/';
> +			strbuf_addstr(&buffer1, name1);
> +			if (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')
> +				strbuf_addch(&buffer1, '/');
>  		}
>
>  		if (name2) {
> -			len2 = strlen(name2);
> -			if (len2 > 0 && name2[len2 - 1] == '/')
> -				len2--;
> -			memcpy(buffer2, name2, len2);
> -			buffer2[len2++] = '/';
> +			strbuf_addstr(&buffer2, name2);
> +			if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
> +				strbuf_addch(&buffer2, '/');

Hi Junio,

That looks much better.
I verified that it compiles and passes the tests on x86_64/Fedora 17.

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
)

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":

------------------------------------
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');






--
NO wt-status.c-			if (*line != '\n' && *line != '\t')
NO wt-status.c:				strbuf_addch(&linebuf, ' ');
--
NO builtin/merge.c-	while ((commit = get_revision(&rev)) != NULL) {
NO builtin/merge.c:		strbuf_addch(&out, '\n');
--
NO builtin/shortlog.c-	if (col != log->wrap)
NO builtin/shortlog.c:		strbuf_addch(sb, '\n');
--
NO dir.c-	if (path->buf[original_len - 1] != '/')
NO dir.c:		strbuf_addch(path, '/');
--
NO path.c-	if (len && path[len-1] != '/')
NO path.c:		strbuf_addch(&buf, '/');
--
NO pretty.c-			if (p != commit->parents)
NO pretty.c:				strbuf_addch(sb, ' ');
--
NO pretty.c-			if (p != commit->parents)
NO pretty.c:				strbuf_addch(sb, ' ');
--
NO pretty.c-	if (pp->fmt != CMIT_FMT_ONELINE && !pp->subject) {
NO pretty.c:		strbuf_addch(sb, '\n');
--
NO pretty.c-	if (pp->fmt != CMIT_FMT_ONELINE)
NO pretty.c:		strbuf_addch(sb, '\n');
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html