git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Brandon Williams <bmwill@google.com>,
	Joachim Durchholz <jo@durchholz.org>,
	Stefan Beller <sbeller@google.com>
Subject: Re: [PATCH] connect.c: handle errors from split_cmdline
Date: Mon, 10 Apr 2017 20:35:54 -0400	[thread overview]
Message-ID: <20170411003554.2tjnn65vfco376kj@sigill.intra.peff.net> (raw)
In-Reply-To: <20170411003023.ynzc76yzdmomxthp@sigill.intra.peff.net>

On Mon, Apr 10, 2017 at 08:30:23PM -0400, Jeff King wrote:

> On Tue, Apr 11, 2017 at 01:23:32AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > 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.

Actually, nevermind. I thought there was an issue with freeing via the
results of basename(), but there isn't. There is a minor memory leak,
but it's best squashed into my original patch, like so:

-- >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.

While we're here, let's also fix the leak when our split
fails (as it turns out, split_cmdline can never return 0, so
this leak wasn't actually triggerable before).

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

diff --git a/connect.c b/connect.c
index 7d65c1c73..380997afd 100644
--- a/connect.c
+++ b/connect.c
@@ -729,18 +729,20 @@ static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
 	} else {
 		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
 			 * referenced by p, hence we do not need ssh_argv
 			 * any longer.
 			 */
 			free(ssh_argv);
-		} else
+		} else {
+			free(p);
 			return;
+		}
 	}
 
 	if (!strcasecmp(variant, "plink") ||
 	    !strcasecmp(variant, "plink.exe"))
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
@@ -426,8 +426,14 @@ test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
 	git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
 	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
 
 counter=0
-- 
2.12.2.952.g759391acc


  reply	other threads:[~2017-04-11  0:36 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-09 19:11 [PATCH 0/2] test: Detect *lots* of bugs by adding non-alnum to trash dir names Ævar Arnfjörð Bjarmason
2017-04-09 19:11 ` [PATCH 1/2] tests: mark tests that fail when the TEST_DIRECTORY is unusual Ævar Arnfjörð Bjarmason
2017-04-09 19:11 ` [PATCH 2/2] test-lib: exhaustively insert non-alnum ASCII into the TRASH_DIRECTORY name Ævar Arnfjörð Bjarmason
2017-04-10  1:47   ` SZEDER Gábor
2017-04-10  8:02     ` Ævar Arnfjörð Bjarmason
2017-04-10 11:19       ` SZEDER Gábor
2017-04-10 11:40         ` Ævar Arnfjörð Bjarmason
2017-04-10 13:38           ` Jeff King
2017-04-10 14:59             ` Joachim Durchholz
2017-04-10 16:57               ` Jeff King
2017-04-10 18:19                 ` Joachim Durchholz
2017-04-10 19:22                   ` Jeff King
2017-04-10 13:43           ` SZEDER Gábor
2017-04-10 23:23   ` Ævar Arnfjörð Bjarmason
2017-04-11  0:30     ` [PATCH] connect.c: handle errors from split_cmdline Jeff King
2017-04-11  0:35       ` Jeff King [this message]
2017-04-11  9:27         ` Ævar Arnfjörð Bjarmason
2017-04-11 10:54           ` Jeff King
2017-04-11 11:06             ` Ævar Arnfjörð Bjarmason
2017-04-17  0:51               ` Junio C Hamano
2017-04-17  0:54               ` Junio C Hamano
2017-04-19 10:59                 ` Ævar Arnfjörð Bjarmason
2017-04-11  1:14     ` [PATCH 2/2] test-lib: exhaustively insert non-alnum ASCII into the TRASH_DIRECTORY name Jeff King
2017-04-11  6:28     ` Joachim Durchholz
2017-04-09 20:37 ` [PATCH 0/2] test: Detect *lots* of bugs by adding non-alnum to trash dir names Joachim Durchholz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170411003554.2tjnn65vfco376kj@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jo@durchholz.org \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).