Re: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug
Eric Sunshine sunsh...@sunshineco.com writes: A bounds checking bug allows the X in -LX to extend one line past the end of file. For example, given a file with 5 lines, -L6 is accepted as valid. Demonstrate this problem. While here, also add tests to check that the remaining cases of X and Y in -LX,Y are handled correctly at and in the vicinity of end-of-file. Signed-off-by: Eric Sunshine sunsh...@sunshineco.com --- t/annotate-tests.sh | 22 ++ 1 file changed, 22 insertions(+) diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh index 3524eaf..02fbbf1 100644 --- a/t/annotate-tests.sh +++ b/t/annotate-tests.sh @@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' ' check_count -L/99/,-3 B 1 B2 1 D 1 ' +# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than +# git-blame sees, hence the last line is actually $(wc...)+1. +test_expect_success 'blame -L X (X == nlines)' ' + n=$(expr $(wc -l file) + 1) + check_count -L$n C 1 +' + +test_expect_failure 'blame -L X (X == nlines + 1)' ' + n=$(expr $(wc -l file) + 2) + test_must_fail $PROG -L$n file +' + test_expect_success 'blame -L X (X nlines)' ' test_must_fail $PROG -L12345 file ' +test_expect_success 'blame -L ,Y (Y == nlines)' ' + n=$(expr $(wc -l file) + 1) + check_count -L,$n A 1 B 1 B1 1 B2 1 A U Thor 1 C 1 D 1 E 1 +' + +test_expect_success 'blame -L ,Y (Y == nlines + 1)' ' + n=$(expr $(wc -l file) + 2) + test_must_fail $PROG -L,$n file +' + This is somewhat curious. Does the problem triggers only on a file that ends with an incomplete line, or the test was inserted at this location only because it was convenient and the problem exists with or without the incomplete final line? I am guessing that it is the latter. Thanks. test_expect_success 'blame -L ,Y (Y nlines)' ' test_must_fail $PROG -L,12345 file ' -- 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: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug
On Mon, Aug 5, 2013 at 3:29 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: A bounds checking bug allows the X in -LX to extend one line past the end of file. For example, given a file with 5 lines, -L6 is accepted as valid. Demonstrate this problem. While here, also add tests to check that the remaining cases of X and Y in -LX,Y are handled correctly at and in the vicinity of end-of-file. Signed-off-by: Eric Sunshine sunsh...@sunshineco.com --- t/annotate-tests.sh | 22 ++ 1 file changed, 22 insertions(+) diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh index 3524eaf..02fbbf1 100644 --- a/t/annotate-tests.sh +++ b/t/annotate-tests.sh @@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' ' check_count -L/99/,-3 B 1 B2 1 D 1 ' +# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than +# git-blame sees, hence the last line is actually $(wc...)+1. +test_expect_success 'blame -L X (X == nlines)' ' + n=$(expr $(wc -l file) + 1) + check_count -L$n C 1 +' + +test_expect_failure 'blame -L X (X == nlines + 1)' ' + n=$(expr $(wc -l file) + 2) + test_must_fail $PROG -L$n file +' + test_expect_success 'blame -L X (X nlines)' ' test_must_fail $PROG -L12345 file ' +test_expect_success 'blame -L ,Y (Y == nlines)' ' + n=$(expr $(wc -l file) + 1) + check_count -L,$n A 1 B 1 B1 1 B2 1 A U Thor 1 C 1 D 1 E 1 +' + +test_expect_success 'blame -L ,Y (Y == nlines + 1)' ' + n=$(expr $(wc -l file) + 2) + test_must_fail $PROG -L,$n file +' + This is somewhat curious. Does the problem triggers only on a file that ends with an incomplete line, or the test was inserted at this location only because it was convenient and the problem exists with or without the incomplete final line? I am guessing that it is the latter. The problem exists with and without the incomplete line. The comment mentioning incomplete line and wc was inserted to explain why it was necessary to add one to the result of wc, which might not otherwise be obvious. The tests were inserted at this location because they are semantically related to the blame -L ,Y (Y nlines) test which already exists in the file (quote just below this response). test_expect_success 'blame -L ,Y (Y nlines)' ' test_must_fail $PROG -L,12345 file ' -- 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: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug
On Mon, Aug 5, 2013 at 3:35 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Mon, Aug 5, 2013 at 3:29 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: A bounds checking bug allows the X in -LX to extend one line past the end of file. For example, given a file with 5 lines, -L6 is accepted as valid. Demonstrate this problem. While here, also add tests to check that the remaining cases of X and Y in -LX,Y are handled correctly at and in the vicinity of end-of-file. Signed-off-by: Eric Sunshine sunsh...@sunshineco.com --- t/annotate-tests.sh | 22 ++ 1 file changed, 22 insertions(+) diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh index 3524eaf..02fbbf1 100644 --- a/t/annotate-tests.sh +++ b/t/annotate-tests.sh @@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' ' check_count -L/99/,-3 B 1 B2 1 D 1 ' +# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than +# git-blame sees, hence the last line is actually $(wc...)+1. +test_expect_success 'blame -L X (X == nlines)' ' + n=$(expr $(wc -l file) + 1) + check_count -L$n C 1 This is somewhat curious. Does the problem triggers only on a file that ends with an incomplete line, or the test was inserted at this location only because it was convenient and the problem exists with or without the incomplete final line? I am guessing that it is the latter. The problem exists with and without the incomplete line. The comment mentioning incomplete line and wc was inserted to explain why it was necessary to add one to the result of wc, which might not otherwise be obvious. Would the comment be clearer if phrased like this? # We want to test -LX where X is the last line of the file, so we need # to compute the number of lines in the file, which normally would be # done via 'wc -l'. In this case, however, the last line of 'file' is # incomplete, so 'wc' reports one fewer than the actual line count. To # adjust for this anomaly, we must add one to the result of 'wc'. -- 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: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug
Eric Sunshine sunsh...@sunshineco.com writes: The problem exists with and without the incomplete line. The comment mentioning incomplete line and wc was inserted to explain why it was necessary to add one to the result of wc, which might not otherwise be obvious. Would the comment be clearer if phrased like this? # We want to test -LX where X is the last line of the file, so we need # to compute the number of lines in the file, which normally would be # done via 'wc -l'. In this case, however, the last line of 'file' is # incomplete, so 'wc' reports one fewer than the actual line count. To # adjust for this anomaly, we must add one to the result of 'wc'. If the patch picked a place where the test vector does not have to involve a file that ends with an incomplete line, the test would not have had to do anything tricky that required such a comment to explain why it is doing something mysterious. Such a change would be much cleaner and may be worth the effort, but I do not think just rewording the comment is worth the bother. I didn't see if there is such a place in the existing test sequence, though. -- 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
[PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug
A bounds checking bug allows the X in -LX to extend one line past the end of file. For example, given a file with 5 lines, -L6 is accepted as valid. Demonstrate this problem. While here, also add tests to check that the remaining cases of X and Y in -LX,Y are handled correctly at and in the vicinity of end-of-file. Signed-off-by: Eric Sunshine sunsh...@sunshineco.com --- t/annotate-tests.sh | 22 ++ 1 file changed, 22 insertions(+) diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh index 3524eaf..02fbbf1 100644 --- a/t/annotate-tests.sh +++ b/t/annotate-tests.sh @@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' ' check_count -L/99/,-3 B 1 B2 1 D 1 ' +# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than +# git-blame sees, hence the last line is actually $(wc...)+1. +test_expect_success 'blame -L X (X == nlines)' ' + n=$(expr $(wc -l file) + 1) + check_count -L$n C 1 +' + +test_expect_failure 'blame -L X (X == nlines + 1)' ' + n=$(expr $(wc -l file) + 2) + test_must_fail $PROG -L$n file +' + test_expect_success 'blame -L X (X nlines)' ' test_must_fail $PROG -L12345 file ' +test_expect_success 'blame -L ,Y (Y == nlines)' ' + n=$(expr $(wc -l file) + 1) + check_count -L,$n A 1 B 1 B1 1 B2 1 A U Thor 1 C 1 D 1 E 1 +' + +test_expect_success 'blame -L ,Y (Y == nlines + 1)' ' + n=$(expr $(wc -l file) + 2) + test_must_fail $PROG -L,$n file +' + test_expect_success 'blame -L ,Y (Y nlines)' ' test_must_fail $PROG -L,12345 file ' -- 1.8.3.4.1120.gc240c48 -- 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
