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

From: Jim Meyering <jim@meyering.net>
Date: Apr 26, 2012, 15:52 UTC
Message-ID: <87d36uxzfw.fsf@rho.meyering.net>
In-Reply-To: <xmqq1unbd2m5.fsf@junio.mtv.corp.google.com>

Junio C Hamano wrote:

Show 73 quoted lines

> Jim Meyering <jim@meyering.net> 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');