threads / patch / 12365

patchFix builtin checkout crashing when given an invalid path

Subject: [PATCH] Fix builtin checkout crashing when given an invalid path

## tl;dr

3 messages between Feb 28, 2008 and Feb 28, 2008. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Alex Riesen· Feb 28, 2008, 16:30 UTC · lore
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Noticed in t2008, which actually passed, but silently removed core-files (I saw segfaults in syslog) and did not properly check the exit code. The change for the t2008 comes as seperate patch, but it should be noted that "! command" is *not* how you check for a command to have failed. It could have crashed.

 builtin-checkout.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Show changes to builtin-checkout.c +4 −0
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 4a4bb8b..9579ff4 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -545,6 +545,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 
 	if (argc) {
 		const char **pathspec = get_pathspec(prefix, argv);
+
+		if (!pathspec)
+			die("invalid path specification");
+
 		/* Checkout paths */
 		if (opts.new_branch || opts.force || opts.merge) {
 			if (argc == 1) {
-- 
1.5.4.3.253.g9f1d5
Alex Riesen· Feb 28, 2008, 16:49 UTC · re: Alex Riesen · lore

[PATCH] Expect the exit code of builtin checkout to be in portable range

This allows crashes to be noticed at least in bash and dash, which put the signal which terminated the command in its exit status.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Alex Riesen, Thu, Feb 28, 2008 17:30:47 +0100:
Show 5 quoted lines
> Noticed in t2008, which actually passed, but silently removed
> core-files (I saw segfaults in syslog) and did not properly check the
> exit code.  The change for the t2008 comes as seperate patch, but it
> should be noted that "! command" is *not* how you check for a command
> to have failed. It could have crashed.

So we'd better check the exit status in failure tests. Like this, for instance

 t/t2008-checkout-subdir.sh |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions(+), 8 deletions(-)
Show changes to t/t2008-checkout-subdir.sh +20 −8
diff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh
index 4a723dc..1295a09 100755
--- a/t/t2008-checkout-subdir.sh
+++ b/t/t2008-checkout-subdir.sh
@@ -67,16 +67,28 @@ test_expect_success 'checkout with simple prefix' '
 
 '
 
-test_expect_success 'relative path outside tree should fail' \
-	'! git checkout HEAD -- ../../Makefile'
+test_expect_success 'relative path outside tree should fail' '
+	git checkout HEAD -- ../../Makefile
+	test $? -gt 0 -a $? -le 128
+'
 
-test_expect_success 'incorrect relative path to file should fail (1)' \
-	'! git checkout HEAD -- ../file0'
+test_expect_success 'incorrect relative path to file should fail (1)' '
+	git checkout HEAD -- ../file0
+	test $? -gt 0 -a $? -le 128
+'
 
-test_expect_success 'incorrect relative path should fail (2)' \
-	'( cd dir1 && ! git checkout HEAD -- ./file0 )'
+test_expect_success 'incorrect relative path should fail (2)' '
+	( cd dir1 && {
+	    git checkout HEAD -- ./file0
+	    test $? -gt 0 -a $? -le 128
+	} )
+'
 
-test_expect_success 'incorrect relative path should fail (3)' \
-	'( cd dir1 && ! git checkout HEAD -- ../../file0 )'
+test_expect_success 'incorrect relative path should fail (3)' '
+	( cd dir1 && {
+	    git checkout HEAD -- ../../file0
+	    test $? -gt 0 -a $? -le 128
+	} )
+'
 
 test_done
-- 
1.5.4.3.253.g9f1d5
Junio C Hamano· Feb 28, 2008, 20:52 UTC · re: Alex Riesen · lore

Re: [PATCH] Expect the exit code of builtin checkout to be in portable range

Alex Riesen <raa.lkml@gmail.com> writes:
Show 15 quoted lines
> This allows crashes to be noticed at least in bash and dash, which put
> the signal which terminated the command in its exit status.
>
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
>
> Alex Riesen, Thu, Feb 28, 2008 17:30:47 +0100:
>> Noticed in t2008, which actually passed, but silently removed
>> core-files (I saw segfaults in syslog) and did not properly check the
>> exit code.  The change for the t2008 comes as seperate patch, but it
>> should be noted that "! command" is *not* how you check for a command
>> to have failed. It could have crashed.
>
> So we'd better check the exit status in failure tests.
> Like this, for instance

Yeah, very well spotted, although I would have liked it if this were caught before it hit 'master'.

I think we want a helper shell function to fix potentially many other uses of "! git-command" construct, so how about doing it this way?

---
 t/t2008-checkout-subdir.sh |    8 ++++----
 t/test-lib.sh              |   17 +++++++++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)
Show changes to 2 files +21 −4

t/t2008-checkout-subdir.sh, t/test-lib.sh

diff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh
index 4a723dc..3e098ab 100755
--- a/t/t2008-checkout-subdir.sh
+++ b/t/t2008-checkout-subdir.sh
@@ -68,15 +68,15 @@ test_expect_success 'checkout with simple prefix' '
 '
 
 test_expect_success 'relative path outside tree should fail' \
-	'! git checkout HEAD -- ../../Makefile'
+	'test_must_fail git checkout HEAD -- ../../Makefile'
 
 test_expect_success 'incorrect relative path to file should fail (1)' \
-	'! git checkout HEAD -- ../file0'
+	'test_must_fail git checkout HEAD -- ../file0'
 
 test_expect_success 'incorrect relative path should fail (2)' \
-	'( cd dir1 && ! git checkout HEAD -- ./file0 )'
+	'( cd dir1 && test_must_fail git checkout HEAD -- ./file0 )'
 
 test_expect_success 'incorrect relative path should fail (3)' \
-	'( cd dir1 && ! git checkout HEAD -- ../../file0 )'
+	'( cd dir1 && test_must_fail git checkout HEAD -- ../../file0 )'
 
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 83889c4..90df619 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -270,6 +270,23 @@ test_expect_code () {
 	echo >&3 ""
 }
 
+# This is not among top-level (test_expect_success | test_expect_failure)
+# but is a prefix that can be used in the test script, like:
+#
+#	test_expect_success 'complain and die' '
+#           do something &&
+#           do something else &&
+#	    test_must_fail git checkout ../outerspace
+#	'
+#
+# Writing this as "! git checkout ../outerspace" is wrong, because
+# the failure could be due to a segv.  We want a controlled failure.
+
+test_must_fail () {
+	"$@"
+	test $? -gt 0 -a $? -le 128
+}
+
 # Most tests can use the created repository, but some may need to create more.
 # Usage: test_create_repo <directory>
 test_create_repo () {

← back to recent threads