Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name
From: Jim Meyering <hidden>
Date: 2016-06-15 22:53:40
Junio C Hamano wrote:
quoted hunk
Jim Meyering [off-list ref] writes:quoted
Marcus Karlsson wrote: ...quoted
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');