Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives

From: Torsten Bögershausen <tboegi@web.de>
Date: Apr 25, 2015, 16:03 UTC
Message-ID: <553BBACA.70704@web.de>
In-Reply-To: <1429914505-325708-2-git-send-email-sandals@crustytoothpaste.net>

On 2015-04-25 00.28, brian m. carlson wrote:

Show 8 quoted lines

> The git_connect function has code to handle plink and tortoiseplink
> specially, as they require different command line arguments from
> OpenSSH.  However, the match was done by checking for "plink"
> case-insensitively in the string, which led to false positives when
> GIT_SSH contained "uplink".  Improve the check by looking for "plink" or
> "tortoiseplink" (or those names suffixed with ".exe") in the final
> component of the path.
>

(I'm not sute if the commit message describes the problem deep enough for readers which are not familar with all the details of the original report): A feature implemented for Windows may break things for e.g. Linux users) The following may read exaggerated, so please read it as a suggestion.

The git_connect function has code to handle plink and tortoiseplink specially, as they require different command line arguments compared to OpenSSH (-P instead of -p, tortoiseplink uses -batch), commit 36ad53ffee6ed. The special handling is only needed for Windows, and a sloppy case-insensitve search for "plink" will trigger that an the extra parameter "-batch" is added to the command line.

This was observed on a Linux system where a command line including "/uplink_deploy/" was used.

There are different ways to improve the situation:
(The following mentions only plink, but assumes that tortoiseplink is handled
 similar)
a) Disable the plink/tortoiseplink special handling on non-Windows systems
b) Tighten the search for plink:
   Allow basename() == plink || !!strcasestr(ssh, "plink.exe")
c) Tighten the search for plink:
   Allow basename() == plink || !!strcasestr(ssh, "plink.exe")
d) Tighten the check for tortoiseplink.
   Today we set "int putty" to true when plink is found, and -batch
   is set when tortoiseplink is not found.
   This fixes the reported bug, but still has the -P problem.
e) Unix users typically use shell scripts and could use GIT_SSH_COMMAND.
   Declare the GIT_SSH as not-well-documented (and to obsolete ?) for non-Windows systems,

This patch implements c): Extract the basename and compare it to plink, plink.exe respective tortoiseplink/tortoiseplink.exe

Note that there is a slight risk of breakage for Windows users: Strings like "myplink" or "plink-0.83" are no longer accepted.

------------- I would probably vote for a), as Unix/Linux/Mac OS users don't use plink/tortoiseplink at all. ------------- What about adding test-cases in t5601, this will ease the documentation later. f:/util/plink /c/util/plink.exe f:/util/tortoiseplink /c/util/tortoiseplink.exe /usr/local/uplink/sshwrapper.sh

Other opinions, other thoughts ?