Re: [PATCH] connect.c: handle errors from split_cmdline

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Apr 11, 2017 at 2:35 AM, Jeff King <peff@peff.net> 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@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

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.