Re: Bug in 'git am' when applying a broken patch
Stefan Beller sbel...@google.com writes: In the hunk header we can learn about the expected lines to read for this hunk and after the hunk we only have 3 possible lines: * it's the next hunk, then the line starts with @@ This is true. * it's a new file, so the line starts with diff --git This is true with s/--git//. * it's the end of the patch, so the line is --\n and the line there after is version number as git describe puts (not sure we want to test on that) This is not true in general, as we do not want to limit git apply to only what git diff produces. You can write anything after a patch and that is still a valid patch. And that anything could be a line that begins with '-', ' ' and '+'; as long as the line numbers in the hunk header are correct, we'd ignore it. So as you said, the change you are responding to is better than nothing, and would only help when you truncate the patch (or break the numbers), but does not protect against arbitrary breakage. One thing we _could_ do is after seeing the end of a message (i.e. we did not see @@ that signals there are more hunks in the current patch, and we did not see diff that signals there are more patches), we keep scanning and declare breakage if we see lines that begin with something that looks like a hunk @@ ... @@. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in 'git am' when applying a broken patch
On Mon, Jun 1, 2015 at 1:23 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: s/enw/new/ Heh, thanks; I wasn't planning to commit this one yet, but why not. Here is with an updated log message and a test. -- 8 -- Subject: [PATCH] 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 input does not have a good way for us to diagnose it as a corrupt patch. We just read two context lines and discard the remainder as cruft, which we must do in order to ignore the e-mail footer. Notice that the patch does not change anything and signal an error. Note that this fix will not help if the hand-edited hunk header were @@ -660,3, +660,2 to include the removal. We would just remove the old text without adding the new one, and treat + some new text and everything after that line as trailing cruft. So it is dubious that this patch alone would help very much in practice, but it may be better than nothing. I agree on this patch being better than nothing, but IMHO we can make the check better. In the hunk header we can learn about the expected lines to read for this hunk and after the hunk we only have 3 possible lines: * it's the next hunk, then the line starts with @@ * it's a new file, so the line starts with diff --git * it's the end of the patch, so the line is --\n and the line there after is version number as git describe puts (not sure we want to test on that) I think this would be a add more safety against missformed patches. Signed-off-by: Junio C Hamano gits...@pobox.com --- builtin/apply.c| 3 +++ t/t4136-apply-check.sh | 13 + 2 files changed, 16 insertions(+) diff --git a/builtin/apply.c b/builtin/apply.c index 6696ea4..606eddd 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -1639,6 +1639,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; diff --git a/t/t4136-apply-check.sh b/t/t4136-apply-check.sh index a321f7c..4b0a374 100755 --- a/t/t4136-apply-check.sh +++ b/t/t4136-apply-check.sh @@ -16,4 +16,17 @@ test_expect_success 'apply --check exits non-zero with unrecognized input' ' EOF ' +test_expect_success 'apply exits non-zero with no-op patch' ' + cat input -\EOF + diff --get a/1 b/1 + index 6696ea4..606eddd 100644 + --- a/1 + +++ b/1 + @@ -1,1 +1,1 @@ +1 + EOF + test_must_fail git apply --stat input + test_must_fail git apply --check input +' + test_done -- 2.4.2-556-g58822d7 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in 'git am' when applying a broken patch
Hi Greg, On Mon, Jun 1, 2015 at 3:54 AM, Greg KH gre...@linuxfoundation.org wrote: On Mon, Jun 01, 2015 at 09:17:59AM +0900, Greg KH wrote: Hi all, I received the patch attached below as part of a submission against the Linux kernel tree. The patch seems to have been hand-edited, and is not correct, and patch verifies this as being a problem: $ patch -p1 --dry-run bad_patch.mbox checking file drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c patch: malformed patch at line 133:skb_put(skb, sizeof(struct ieee80211_authentication)); But git will actually apply it: $ git am -s bad_patch.mbox Applying: staging: rtl8192u: ieee80211: Fix sparse endianness warnings But, there's nothing in the patch at all except the commit message: $ git show HEAD commit f6643dfef5b701db86f23be9ce6fb5b3bafe76b6 Author: Gaston Gonzalez gasc...@gmail.com Date: Sun May 31 12:17:48 2015 -0300 staging: rtl8192u: ieee80211: Fix sparse endianness warnings Fix the following sparse warnings: drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32: warning: incorrect type in assignment (different base types) drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32: expected restricted __le16 [usertype] frame_ctl drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:got int drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50: warning: invalid assignment: |= drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:left side has type restricted __le16 drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:right side has type int Signed-off-by: Gaston Gonzalez gasc...@gmail.com Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org $ git diff HEAD^ $ Any ideas what is going on here? Shouldn't 'git am' have failed? Oh, I'm using git version 2.4.2 right now. I've asked Gaston for the original patch to verify before he hand-edited it, to verify that git wasn't creating something wrong here, as well. Gaston sent me his original patch, before he edited it, and it was correct, so git is correctly creating the patch, which is good. So it's just a 'git am' issue with a broken patch file. Yeah, git am is calling 'git apply --index' on the attached patch and 'git apply' doesn't apply it, doesn't warn and exits with code 0. Thanks, Christian. --- drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c index d2e8b12..0477ba1 100644 --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c @@ -660,2 +660,2 @@ inline struct sk_buff *ieee80211_authentication_req(struct ieee80211_network *be auth = (struct ieee80211_authentication *) skb_put(skb, sizeof(struct ieee80211_authentication)); - auth-header.frame_ctl = IEEE80211_STYPE_AUTH; - if (challengelen) auth-header.frame_ctl |= IEEE80211_FCTL_WEP; + auth-header.frame_ctl = cpu_to_le16(IEEE80211_STYPE_AUTH); + if (challengelen) + auth-header.frame_ctl |= cpu_to_le16(IEEE80211_FCTL_WEP); auth-header.duration_id = 0x013a; //FIXME -- 2.1.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Bug in 'git am' when applying a broken patch
Greg KH gre...@linuxfoundation.org writes: But, there's nothing in the patch at all except the commit message: $ git show HEAD ... Any ideas what is going on here? Shouldn't 'git am' have failed? Yes. The patch reads like this: --- drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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,2 +660,2 @@ inline struct sk_buff *ieee80211_authentic... auth = (struct ieee80211_authentication *) skb_put(skb, sizeof(struct ieee80211_authentication)); - auth-header.frame_ctl = IEEE80211_STYPE_AUTH; - if (challengelen) auth-header.frame_ctl |= IEEE80211_FCTL_WEP; + auth-header.frame_ctl = cpu_to_le16(IEEE80211_STYPE_AUTH); + if (challengelen) + auth-header.frame_ctl |= cpu_to_le16(IEEE80211_FCTL_WEP); auth-header.duration_id = 0x013a; //FIXME -- 2.1.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel 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 (which is a blank line, not even a lone SP on it) 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in 'git am' when applying a broken patch
On Mon, Jun 1, 2015 at 2:58 PM, Junio C Hamano gits...@pobox.com wrote: 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 s/enw/new/ 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 gits...@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; -- -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in 'git am' when applying a broken patch
Junio C Hamano gits...@pobox.com writes: 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 gits...@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; -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in 'git am' when applying a broken patch
Eric Sunshine sunsh...@sunshineco.com writes: s/enw/new/ Heh, thanks; I wasn't planning to commit this one yet, but why not. Here is with an updated log message and a test. -- 8 -- Subject: [PATCH] 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 input does not have a good way for us to diagnose it as a corrupt patch. We just read two context lines and discard the remainder as cruft, which we must do in order to ignore the e-mail footer. Notice that the patch does not change anything and signal an error. Note that this fix will not help if the hand-edited hunk header were @@ -660,3, +660,2 to include the removal. We would just remove the old text without adding the new one, and treat + some new text and everything after that line as trailing cruft. So it is dubious that this patch alone would help very much in practice, but it may be better than nothing. Signed-off-by: Junio C Hamano gits...@pobox.com --- builtin/apply.c| 3 +++ t/t4136-apply-check.sh | 13 + 2 files changed, 16 insertions(+) diff --git a/builtin/apply.c b/builtin/apply.c index 6696ea4..606eddd 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -1639,6 +1639,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; diff --git a/t/t4136-apply-check.sh b/t/t4136-apply-check.sh index a321f7c..4b0a374 100755 --- a/t/t4136-apply-check.sh +++ b/t/t4136-apply-check.sh @@ -16,4 +16,17 @@ test_expect_success 'apply --check exits non-zero with unrecognized input' ' EOF ' +test_expect_success 'apply exits non-zero with no-op patch' ' + cat input -\EOF + diff --get a/1 b/1 + index 6696ea4..606eddd 100644 + --- a/1 + +++ b/1 + @@ -1,1 +1,1 @@ +1 + EOF + test_must_fail git apply --stat input + test_must_fail git apply --check input +' + test_done -- 2.4.2-556-g58822d7 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in 'git am' when applying a broken patch
On Mon, Jun 01, 2015 at 01:23:26PM -0700, Junio C Hamano wrote: Eric Sunshine sunsh...@sunshineco.com writes: s/enw/new/ Heh, thanks; I wasn't planning to commit this one yet, but why not. Well, it's not good to apply a commit with no actual commit. That never a good thing, and was the thing that really confused me about this issue. Here is with an updated log message and a test. -- 8 -- Subject: [PATCH] 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 input does not have a good way for us to diagnose it as a corrupt patch. We just read two context lines and discard the remainder as cruft, which we must do in order to ignore the e-mail footer. Notice that the patch does not change anything and signal an error. Note that this fix will not help if the hand-edited hunk header were @@ -660,3, +660,2 to include the removal. We would just remove the old text without adding the new one, and treat + some new text and everything after that line as trailing cruft. So it is dubious that this patch alone would help very much in practice, but it may be better than nothing. Signed-off-by: Junio C Hamano gits...@pobox.com --- builtin/apply.c| 3 +++ t/t4136-apply-check.sh | 13 + 2 files changed, 16 insertions(+) Looks good to me, thanks for fixing this, much appreciated. greg k-h -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug in 'git am' when applying a broken patch
On Mon, Jun 01, 2015 at 09:17:59AM +0900, Greg KH wrote: Hi all, I received the patch attached below as part of a submission against the Linux kernel tree. The patch seems to have been hand-edited, and is not correct, and patch verifies this as being a problem: $ patch -p1 --dry-run bad_patch.mbox checking file drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c patch: malformed patch at line 133:skb_put(skb, sizeof(struct ieee80211_authentication)); But git will actually apply it: $ git am -s bad_patch.mbox Applying: staging: rtl8192u: ieee80211: Fix sparse endianness warnings But, there's nothing in the patch at all except the commit message: $ git show HEAD commit f6643dfef5b701db86f23be9ce6fb5b3bafe76b6 Author: Gaston Gonzalez gasc...@gmail.com Date: Sun May 31 12:17:48 2015 -0300 staging: rtl8192u: ieee80211: Fix sparse endianness warnings Fix the following sparse warnings: drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32: warning: incorrect type in assignment (different base types) drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32: expected restricted __le16 [usertype] frame_ctl drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:663:32:got int drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50: warning: invalid assignment: |= drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:left side has type restricted __le16 drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c:664:50:right side has type int Signed-off-by: Gaston Gonzalez gasc...@gmail.com Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org $ git diff HEAD^ $ Any ideas what is going on here? Shouldn't 'git am' have failed? Oh, I'm using git version 2.4.2 right now. I've asked Gaston for the original patch to verify before he hand-edited it, to verify that git wasn't creating something wrong here, as well. Gaston sent me his original patch, before he edited it, and it was correct, so git is correctly creating the patch, which is good. So it's just a 'git am' issue with a broken patch file. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
