| Message ID | 20191008173843.GC74671@cat (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] range-diff: don't segfault with mode-only changes | expand |
Hi Thomas, On Tue, 8 Oct 2019, Thomas Gummerer wrote: > In ef283b3699 ("apply: make parse_git_diff_header public", 2019-07-11) > the 'parse_git_diff_header' function was made public and useable by > callers outside of apply.c. > > However it was missed that its (then) only caller, 'find_header' did > some error handling, and completing 'struct patch' appropriately. > > range-diff then started using this function, and tried to handle this > appropriately itself, but fell short in some cases. This in turn > would lead to range-diff segfaulting when there are mode-only changes > in a range. > > Move the error handling and completing of the struct into the > 'parse_git_diff_header' function, so other callers can take advantage > of it. This fixes the segfault in 'git range-diff'. > > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de> Ciao, Dscho > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > --- > > Thanks Junio and Dscho for your reviews. I decided to lift the whole > error handling behaviour from find_header into parse_git_diff_header, > instead of just filling the two names with xstrdup(def_name) if > (!old_name && !new_name && !!def_name). I think the additional > information presented there can be useful. For example we would have > gotten some "error: git diff header lacks filename information" > instead of a segfault for the problem described in > https://public-inbox.org/git/20191002141615.GB17916@kitsune.suse.cz/T/#me576615d7a151cf2ed46186c482fbd88f9959914. > > Dscho, I didn't re-use your test case here as I had already written > one, and think what I have is slightly nicer in that it follows what > most other range-diff tests do in using the fast-exported history. It > also expands the test coverage slightly, as we currently don't have > any coverage of the mode-change header, but will with this test. > > The downside is of course that the fast export script is harder to > understand than the test you had, at least for me, but I think the > tradeoff of having the additional test coverage, and having it similar > to the rest of the test script is worth it. If you strongly prefer > your test though I'm not going to be unhappy to use that :) > > apply.c | 43 +++++++++++++++++++++--------------------- > t/t3206-range-diff.sh | 40 +++++++++++++++++++++++++++++++++++++++ > t/t3206/history.export | 31 +++++++++++++++++++++++++++++- > 3 files changed, 92 insertions(+), 22 deletions(-) > > diff --git a/apply.c b/apply.c > index 57a61f2881..f8a046a6a5 100644 > --- a/apply.c > +++ b/apply.c > @@ -1361,11 +1361,32 @@ int parse_git_diff_header(struct strbuf *root, > if (check_header_line(*linenr, patch)) > return -1; > if (res > 0) > - return offset; > + goto done; > break; > } > } > > +done: > + if (!patch->old_name && !patch->new_name) { > + if (!patch->def_name) { > + error(Q_("git diff header lacks filename information when removing " > + "%d leading pathname component (line %d)", > + "git diff header lacks filename information when removing " > + "%d leading pathname components (line %d)", > + parse_hdr_state.p_value), > + parse_hdr_state.p_value, *linenr); > + return -128; > + } > + patch->old_name = xstrdup(patch->def_name); > + patch->new_name = xstrdup(patch->def_name); > + } > + if ((!patch->new_name && !patch->is_delete) || > + (!patch->old_name && !patch->is_new)) { > + error(_("git diff header lacks filename information " > + "(line %d)"), *linenr); > + return -128; > + } > + patch->is_toplevel_relative = 1; > return offset; > } > > @@ -1546,26 +1567,6 @@ static int find_header(struct apply_state *state, > return -128; > if (git_hdr_len <= len) > continue; > - if (!patch->old_name && !patch->new_name) { > - if (!patch->def_name) { > - error(Q_("git diff header lacks filename information when removing " > - "%d leading pathname component (line %d)", > - "git diff header lacks filename information when removing " > - "%d leading pathname components (line %d)", > - state->p_value), > - state->p_value, state->linenr); > - return -128; > - } > - patch->old_name = xstrdup(patch->def_name); > - patch->new_name = xstrdup(patch->def_name); > - } > - if ((!patch->new_name && !patch->is_delete) || > - (!patch->old_name && !patch->is_new)) { > - error(_("git diff header lacks filename information " > - "(line %d)"), state->linenr); > - return -128; > - } > - patch->is_toplevel_relative = 1; > *hdrsize = git_hdr_len; > return offset; > } > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > index ec548654ce..5b87fead2e 100755 > --- a/t/t3206-range-diff.sh > +++ b/t/t3206-range-diff.sh > @@ -226,6 +226,46 @@ test_expect_success 'renamed file' ' > test_cmp expected actual > ' > > +test_expect_success 'file with mode only change' ' > + git range-diff --no-color --submodule=log topic...mode-only-change >actual && > + sed s/Z/\ /g >expected <<-EOF && > + 1: fccce22 ! 1: 4d39cb3 s/4/A/ > + @@ Metadata > + ZAuthor: Thomas Rast <trast@inf.ethz.ch> > + Z > + Z ## Commit message ## > + - s/4/A/ > + + s/4/A/ + add other-file > + Z > + Z ## file ## > + Z@@ > + @@ file > + Z A > + Z 6 > + Z 7 > + + > + + ## other-file (new) ## > + 2: 147e64e ! 2: 26c107f s/11/B/ > + @@ Metadata > + ZAuthor: Thomas Rast <trast@inf.ethz.ch> > + Z > + Z ## Commit message ## > + - s/11/B/ > + + s/11/B/ + mode change other-file > + Z > + Z ## file ## > + Z@@ file: A > + @@ file: A > + Z 12 > + Z 13 > + Z 14 > + + > + + ## other-file (mode change 100644 => 100755) ## > + 3: a63e992 = 3: 4c1e0f5 s/12/B/ > + EOF > + test_cmp expected actual > +' > + > test_expect_success 'file added and later removed' ' > git range-diff --no-color --submodule=log topic...added-removed >actual && > sed s/Z/\ /g >expected <<-EOF && > diff --git a/t/t3206/history.export b/t/t3206/history.export > index 7bb3814962..4c808e5b3b 100644 > --- a/t/t3206/history.export > +++ b/t/t3206/history.export > @@ -55,7 +55,7 @@ A > 19 > 20 > > -commit refs/heads/topic > +commit refs/heads/mode-only-change > mark :4 > author Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200 > committer Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200 > @@ -678,3 +678,32 @@ s/12/B/ > from :55 > M 100644 :9 renamed-file > > +commit refs/heads/mode-only-change > +mark :57 > +author Thomas Rast <trast@inf.ethz.ch> 1374485024 +0200 > +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473767 +0100 > +data 24 > +s/4/A/ + add other-file > +from :4 > +M 100644 :5 file > +M 100644 :49 other-file > + > +commit refs/heads/mode-only-change > +mark :58 > +author Thomas Rast <trast@inf.ethz.ch> 1374485036 +0200 > +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100 > +data 33 > +s/11/B/ + mode change other-file > +from :57 > +M 100644 :7 file > +M 100755 :49 other-file > + > +commit refs/heads/mode-only-change > +mark :59 > +author Thomas Rast <trast@inf.ethz.ch> 1374485044 +0200 > +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100 > +data 8 > +s/12/B/ > +from :58 > +M 100644 :9 file > + > -- > 2.23.0.501.gb744c3af07 > >
On Tue, Oct 08, 2019 at 06:38:43PM +0100, Thomas Gummerer wrote: > In ef283b3699 ("apply: make parse_git_diff_header public", 2019-07-11) > the 'parse_git_diff_header' function was made public and useable by > callers outside of apply.c. > > However it was missed that its (then) only caller, 'find_header' did > some error handling, and completing 'struct patch' appropriately. > > range-diff then started using this function, and tried to handle this > appropriately itself, but fell short in some cases. This in turn > would lead to range-diff segfaulting when there are mode-only changes > in a range. > > Move the error handling and completing of the struct into the > 'parse_git_diff_header' function, so other callers can take advantage > of it. This fixes the segfault in 'git range-diff'. > > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> This patch also makes git work again for the originally problematic usecase. Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks for your quick reaction to my bug report, Uwe Kleine-König
diff --git a/apply.c b/apply.c index 57a61f2881..f8a046a6a5 100644 --- a/apply.c +++ b/apply.c @@ -1361,11 +1361,32 @@ int parse_git_diff_header(struct strbuf *root, if (check_header_line(*linenr, patch)) return -1; if (res > 0) - return offset; + goto done; break; } } +done: + if (!patch->old_name && !patch->new_name) { + if (!patch->def_name) { + error(Q_("git diff header lacks filename information when removing " + "%d leading pathname component (line %d)", + "git diff header lacks filename information when removing " + "%d leading pathname components (line %d)", + parse_hdr_state.p_value), + parse_hdr_state.p_value, *linenr); + return -128; + } + patch->old_name = xstrdup(patch->def_name); + patch->new_name = xstrdup(patch->def_name); + } + if ((!patch->new_name && !patch->is_delete) || + (!patch->old_name && !patch->is_new)) { + error(_("git diff header lacks filename information " + "(line %d)"), *linenr); + return -128; + } + patch->is_toplevel_relative = 1; return offset; } @@ -1546,26 +1567,6 @@ static int find_header(struct apply_state *state, return -128; if (git_hdr_len <= len) continue; - if (!patch->old_name && !patch->new_name) { - if (!patch->def_name) { - error(Q_("git diff header lacks filename information when removing " - "%d leading pathname component (line %d)", - "git diff header lacks filename information when removing " - "%d leading pathname components (line %d)", - state->p_value), - state->p_value, state->linenr); - return -128; - } - patch->old_name = xstrdup(patch->def_name); - patch->new_name = xstrdup(patch->def_name); - } - if ((!patch->new_name && !patch->is_delete) || - (!patch->old_name && !patch->is_new)) { - error(_("git diff header lacks filename information " - "(line %d)"), state->linenr); - return -128; - } - patch->is_toplevel_relative = 1; *hdrsize = git_hdr_len; return offset; } diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index ec548654ce..5b87fead2e 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -226,6 +226,46 @@ test_expect_success 'renamed file' ' test_cmp expected actual ' +test_expect_success 'file with mode only change' ' + git range-diff --no-color --submodule=log topic...mode-only-change >actual && + sed s/Z/\ /g >expected <<-EOF && + 1: fccce22 ! 1: 4d39cb3 s/4/A/ + @@ Metadata + ZAuthor: Thomas Rast <trast@inf.ethz.ch> + Z + Z ## Commit message ## + - s/4/A/ + + s/4/A/ + add other-file + Z + Z ## file ## + Z@@ + @@ file + Z A + Z 6 + Z 7 + + + + ## other-file (new) ## + 2: 147e64e ! 2: 26c107f s/11/B/ + @@ Metadata + ZAuthor: Thomas Rast <trast@inf.ethz.ch> + Z + Z ## Commit message ## + - s/11/B/ + + s/11/B/ + mode change other-file + Z + Z ## file ## + Z@@ file: A + @@ file: A + Z 12 + Z 13 + Z 14 + + + + ## other-file (mode change 100644 => 100755) ## + 3: a63e992 = 3: 4c1e0f5 s/12/B/ + EOF + test_cmp expected actual +' + test_expect_success 'file added and later removed' ' git range-diff --no-color --submodule=log topic...added-removed >actual && sed s/Z/\ /g >expected <<-EOF && diff --git a/t/t3206/history.export b/t/t3206/history.export index 7bb3814962..4c808e5b3b 100644 --- a/t/t3206/history.export +++ b/t/t3206/history.export @@ -55,7 +55,7 @@ A 19 20 -commit refs/heads/topic +commit refs/heads/mode-only-change mark :4 author Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200 committer Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200 @@ -678,3 +678,32 @@ s/12/B/ from :55 M 100644 :9 renamed-file +commit refs/heads/mode-only-change +mark :57 +author Thomas Rast <trast@inf.ethz.ch> 1374485024 +0200 +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473767 +0100 +data 24 +s/4/A/ + add other-file +from :4 +M 100644 :5 file +M 100644 :49 other-file + +commit refs/heads/mode-only-change +mark :58 +author Thomas Rast <trast@inf.ethz.ch> 1374485036 +0200 +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100 +data 33 +s/11/B/ + mode change other-file +from :57 +M 100644 :7 file +M 100755 :49 other-file + +commit refs/heads/mode-only-change +mark :59 +author Thomas Rast <trast@inf.ethz.ch> 1374485044 +0200 +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100 +data 8 +s/12/B/ +from :58 +M 100644 :9 file +
In ef283b3699 ("apply: make parse_git_diff_header public", 2019-07-11) the 'parse_git_diff_header' function was made public and useable by callers outside of apply.c. However it was missed that its (then) only caller, 'find_header' did some error handling, and completing 'struct patch' appropriately. range-diff then started using this function, and tried to handle this appropriately itself, but fell short in some cases. This in turn would lead to range-diff segfaulting when there are mode-only changes in a range. Move the error handling and completing of the struct into the 'parse_git_diff_header' function, so other callers can take advantage of it. This fixes the segfault in 'git range-diff'. Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- Thanks Junio and Dscho for your reviews. I decided to lift the whole error handling behaviour from find_header into parse_git_diff_header, instead of just filling the two names with xstrdup(def_name) if (!old_name && !new_name && !!def_name). I think the additional information presented there can be useful. For example we would have gotten some "error: git diff header lacks filename information" instead of a segfault for the problem described in https://public-inbox.org/git/20191002141615.GB17916@kitsune.suse.cz/T/#me576615d7a151cf2ed46186c482fbd88f9959914. Dscho, I didn't re-use your test case here as I had already written one, and think what I have is slightly nicer in that it follows what most other range-diff tests do in using the fast-exported history. It also expands the test coverage slightly, as we currently don't have any coverage of the mode-change header, but will with this test. The downside is of course that the fast export script is harder to understand than the test you had, at least for me, but I think the tradeoff of having the additional test coverage, and having it similar to the rest of the test script is worth it. If you strongly prefer your test though I'm not going to be unhappy to use that :) apply.c | 43 +++++++++++++++++++++--------------------- t/t3206-range-diff.sh | 40 +++++++++++++++++++++++++++++++++++++++ t/t3206/history.export | 31 +++++++++++++++++++++++++++++- 3 files changed, 92 insertions(+), 22 deletions(-)