[PATCH] connect.c: handle errors from split_cmdline

From: Jeff King <peff@peff.net>
Date: Apr 11, 2017, 00:30 UTC
Message-ID: <20170411003023.ynzc76yzdmomxthp@sigill.intra.peff.net>
In-Reply-To: <CACBZZX7OfM-zivJAQMXdNarHDjAhzQhqGNZNs2QqDUyOo3AA0g@mail.gmail.com>

On Tue, Apr 11, 2017 at 01:23:32AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 7 quoted lines

> There's one segfault in there:
> 
> $ ./t5601-clone.sh --root="xtmp.$(perl -e 'print chr 39')" -v -i -d
> [...]
> Cloning into 'ssh-bracket-clone-plink-4'...
> Segmentation fault
> not ok 45 - single quoted plink.exe in GIT_SSH_COMMAND

Here's a fix for that one. I think there are a few other memory irregularities in that function, too. I'll send another patch in a minute, but I wanted to get this out in case you were working on it, too.

-- >8 --
Subject: [PATCH] connect.c: handle errors from split_cmdline

Commit e9d9a8a4d (connect: handle putty/plink also in GIT_SSH_COMMAND, 2017-01-02) added a call to split_cmdline(), but checks only for a non-zero return to see if we got any output. Since the function returns negative values (and a NULL argv) on error, we end up dereferencing NULL and segfaulting.

Arguably we could report on the parsing error here, but it's probably not worth it. This is a best-effort attempt to see if we are using plink. So we can simply return here with "no, it wasn't plink" and let the shell actually complain about the bogus quoting.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 connect.c        | 2 +-
 t/t5601-clone.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index 7d65c1c73..6f2281ab0 100644
--- a/connect.c
+++ b/connect.c
@@ -730,7 +730,7 @@ static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
 		const char **ssh_argv;
 
 		p = xstrdup(ssh_command);
-		if (split_cmdline(p, &ssh_argv)) {
+		if (split_cmdline(p, &ssh_argv) > 0) {
 			variant = basename((char *)ssh_argv[0]);
 			/*
 			 * At this point, variant points into the buffer
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index b52b8acf9..9c56f771b 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -427,6 +427,12 @@ test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
 	expect_ssh "-batch -P 123" myhost src
 '
 
+test_expect_success 'clean failure on broken quoting' '
+	test_must_fail \
+		env GIT_SSH_COMMAND="${SQ}plink.exe -v" \
+		git clone "[myhost:123]:src" sq-failure
+'
+
 # Reset the GIT_SSH environment variable for clone tests.
 setup_ssh_wrapper

-- 
2.12.2.952.g759391acc