Re: Bug in 'git am' when applying a broken patch

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2015, 18:58 UTC
Message-ID: <xmqqd21f5k7w.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqwpzn5lht.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

> It claims that it has only 2 lines in the hunk, so "git apply"
> parses the hunk that begins at line 660 as such:
>
>     @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...
>             auth = (struct ieee80211_authentication *)
>                     skb_put(skb, sizeof(struct ieee80211_authentication));
>
> And then seeing that the next line does not begin with "@@ -", it
> says "OK, the remainder is a cruft after the patch" and discards
> the rest (which it must be capable of, to ignore "-- ", "2.1.4",
> "devel mailing list", etc.)
>
> There is some safety against not finding a correct patch header
> (i.e. "diff --git" line) by detecting a lone "@@ -" while parsing
> the patch stream, but there is no logic implemented to detect this
> kind of breakage in the code.

For this particular case, it is tempting to say "if a hunk does not have any +/- line, that is clearly bogus", but the breakage could have been like this, telling Git to remove a line without doing anything else.

    diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/...
    index d2e8b12..0477ba1 100644
    --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
    +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
    @@ -660,4 +660,4 @@ inline struct sk_buff *ieee80211_authentication_...
            auth = (struct ieee80211_authentication *)
                    skb_put(skb, sizeof(struct ieee80211_authentication));

    -	auth->header.frame_ctl = IEEE80211_STYPE_AUTH;

So "a no-op hunk is suspicious" may be a good criterion to make "git apply" barf and error out, but that alone would not be a foolproof solution to protect us against a hand-edited patch.

-- >8 --
Subject: apply: reject a hunk that does not do anything

A hunk like this in a hand-edited patch without correctly adjusting the line counts:

     @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic...
             auth = (struct ieee80211_authentication *)
                     skb_put(skb, sizeof(struct ieee80211_authentication));
     -       some old text
     +       some new text
     --
     2.1.0

     dev mailing list

at the end of the patch does not have a good way for us to diagnose it as corrupt patch. We just read two lines and discard the remainder as cruft, which we must do in order to ignore the e-mail footer.

If the hand-edited hunk header were "@@ -660,3, +660,2", this fix will not help---we would just remove the old text without adding the enw one, and treat "+ some new text" and everything after that line as trailing cruft. So it is dubious that this patch would help very much in practice, but it is better than nothing ;-)

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index 146be97..54aba4e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1638,6 +1638,9 @@ static int parse_fragment(const char *line, unsigned long size,
 	}
 	if (oldlines || newlines)
 		return -1;
+	if (!deleted && !added)
+		return -1;
+
 	fragment->leading = leading;
 	fragment->trailing = trailing;