Re: [PATCH v2] range-diff: don't segfault with mode-only changes
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
> Signed-off-by: Thomas Gummerer
This patch also makes git work again for the originally problematic
usecase.
Tested-by: Uwe Kleine-König
Thanks for your quick reaction to my bug report,
Uwe Kleine-König
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH v2] range-diff: don't segfault with mode-only changes
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
Acked-by: Johannes Schindelin
Ciao,
Dscho
> Signed-off-by: Thomas Gummerer
> ---
>
> 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.gb17...@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 -
[PATCH v2] range-diff: don't segfault with mode-only changes
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
Signed-off-by: Thomas Gummerer
---
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.gb17...@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);
-
