On Tue, Apr 11, 2017 at 2:35 AM, Jeff King <peff@xxxxxxxx> wrote:
> 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@xxxxxxxxx>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> 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
Thanks. That makes it work.
Junio: If you're not in some rush to pick this up I'll take this, fix
up a bunch of other bugs & tests failures on odd --root directories
and submit this and Jeff's \r patch (after adding tests etc) along
with it.