Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name
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');
Jim Meyering
