| Message ID | b3384880c7201d65adb7341ce23386d578e69193.1586960921.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | range-diff: fix a crash in parsing git-log output | expand |
On Wed, Apr 15, 2020 at 02:28:41PM +0000, Vasil Dimov via GitGitGadget wrote: > From: Vasil Dimov <vd@FreeBSD.org> > > If the supplied integer for "precisoin" is negative in s/precisoin/precision > `"%.*s", len, line` then it is ignored. So the current code is > equivalent to just `"%s", line` because it is executed only if > `len` is negative. > > Fix this by saving the value of `len` before overwriting it with the > return value of `parse_git_diff_header()`. > > Signed-off-by: Vasil Dimov <vd@FreeBSD.org> > --- > range-diff.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/range-diff.c b/range-diff.c > index 5cc920be391..40af0862818 100644 > --- a/range-diff.c > +++ b/range-diff.c > @@ -123,16 +123,19 @@ static int read_patches(const char *range, struct string_list *list, > struct patch patch = { 0 }; > struct strbuf root = STRBUF_INIT; > int linenr = 0; > + int orig_len; Any reason to not assign this to 'len' up here? > > in_header = 0; > strbuf_addch(&buf, '\n'); > if (!util->diff_offset) > util->diff_offset = buf.len; > line[len - 1] = '\n'; > + orig_len = len; > len = parse_git_diff_header(&root, &linenr, 0, line, > len, size, &patch); OK, so we cut up the line by placing a NL at len, and then feed it to 'parse_git_diff_header' which will tell us the length of the thing that it parsed, or give a negative value if it couldn't parse... > if (len < 0) > - die(_("could not parse git header '%.*s'"), (int)len, line); > + die(_("could not parse git header '%.*s'"), > + orig_len, line); ...and then you restore the original length and print it out here. It seems like this error is now misleading though, because the line is already modified at the point that the newline was inserted. > strbuf_addstr(&buf, " ## "); > if (patch.is_new > 0) > strbuf_addf(&buf, "%s (new)", patch.new_name); > -- > gitgitgadget Thanks, Taylor
On Wed, Apr 15, 2020 at 10:20:35 -0600, Taylor Blau wrote: > On Wed, Apr 15, 2020 at 02:28:41PM +0000, Vasil Dimov via GitGitGadget wrote: > > From: Vasil Dimov <vd@FreeBSD.org> > > > > If the supplied integer for "precisoin" is negative in > > s/precisoin/precision Fixed in v2. > > `"%.*s", len, line` then it is ignored. So the current code is > > equivalent to just `"%s", line` because it is executed only if > > `len` is negative. > > > > Fix this by saving the value of `len` before overwriting it with the > > return value of `parse_git_diff_header()`. > > > > Signed-off-by: Vasil Dimov <vd@FreeBSD.org> > > --- > > range-diff.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/range-diff.c b/range-diff.c > > index 5cc920be391..40af0862818 100644 > > --- a/range-diff.c > > +++ b/range-diff.c > > @@ -123,16 +123,19 @@ static int read_patches(const char *range, struct string_list *list, > > struct patch patch = { 0 }; > > struct strbuf root = STRBUF_INIT; > > int linenr = 0; > > + int orig_len; > > Any reason to not assign this to 'len' up here? I believe that assigning it just before len is changed and that grouping all usage of the new variable close to each other makes the code more readable. Ideally it would also be defined below. > > in_header = 0; > > strbuf_addch(&buf, '\n'); > > if (!util->diff_offset) > > util->diff_offset = buf.len; > > line[len - 1] = '\n'; > > + orig_len = len; > > len = parse_git_diff_header(&root, &linenr, 0, line, > > len, size, &patch); > > OK, so we cut up the line by placing a NL at len, and then feed it to > 'parse_git_diff_header' which will tell us the length of the thing that > it parsed, or give a negative value if it couldn't parse... > > > if (len < 0) > > - die(_("could not parse git header '%.*s'"), (int)len, line); > > + die(_("could not parse git header '%.*s'"), > > + orig_len, line); > > ...and then you restore the original length and print it out here. It > seems like this error is now misleading though, because the line is > already modified at the point that the newline was inserted. [...] It was '\0' before we overwrote it with '\n': 89 len = find_end_of_line(line, size); 90 line[len - 1] = '\0'; `line` points to a buffer of the entire output of `git log`, with many newlines in it. In the beginning of the loop we overwrite the first new line char in the buffer with '\0', then we restore it to '\n' and eventually advance the pointer to the start of the next line. I think that the intention of this code is to print only one line (the current one).
diff --git a/range-diff.c b/range-diff.c index 5cc920be391..40af0862818 100644 --- a/range-diff.c +++ b/range-diff.c @@ -123,16 +123,19 @@ static int read_patches(const char *range, struct string_list *list, struct patch patch = { 0 }; struct strbuf root = STRBUF_INIT; int linenr = 0; + int orig_len; in_header = 0; strbuf_addch(&buf, '\n'); if (!util->diff_offset) util->diff_offset = buf.len; line[len - 1] = '\n'; + orig_len = len; len = parse_git_diff_header(&root, &linenr, 0, line, len, size, &patch); if (len < 0) - die(_("could not parse git header '%.*s'"), (int)len, line); + die(_("could not parse git header '%.*s'"), + orig_len, line); strbuf_addstr(&buf, " ## "); if (patch.is_new > 0) strbuf_addf(&buf, "%s (new)", patch.new_name);