Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives
On Sat, Apr 25, 2015 at 06:03:22PM +0200, Torsten Bögershausen wrote: > (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 can certainly expand the commit message when I reroll. > - > I would probably vote for a), as Unix/Linux/Mac OS users don't use > plink/tortoiseplink > at all. I have putty on my system: vauxhall ok % uname -a && which plink Linux vauxhall 3.16.0-4-amd64 #1 SMP Debian 3.16.7-ckt7-1 (2015-03-01) x86_64 GNU/Linux /usr/bin/plink While it's clearly not very common to use putty on Unix systems, it certainly is possible and it would need to work the same way. > - > 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 It looks like there's already a framework for that, so sure, I can add some tests. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives
On 2015-04-25 00.28, brian m. carlson wrote: > 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 ? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives
On Fri, Apr 24, 2015 at 03:46:30PM -0700, Pete Harlan wrote: > On Fri, Apr 24, 2015 at 3:28 PM, brian m. carlson > wrote: > > 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 > > Perhaps s/case-insensitively/anywhere/? > > It's not important that it ignored case (your change ignores it too), > it's that it was too lenient about its context. Yes, I don't know what I was thinking. I'll wait a bit to see if there are any more comments and then reroll. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives
On Fri, Apr 24, 2015 at 3:28 PM, brian m. carlson
wrote:
> 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
Perhaps s/case-insensitively/anywhere/?
It's not important that it ignored case (your change ignores it too),
it's that it was too lenient about its context.
--Pete
> 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.
>
> Signed-off-by: brian m. carlson
> ---
> connect.c | 18 +++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index 749a07b..c0144d8 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -724,7 +724,7 @@ struct child_process *git_connect(int fd[2], const char
> *url,
> conn->in = conn->out = -1;
> if (protocol == PROTO_SSH) {
> const char *ssh;
> - int putty;
> + int putty, tortoiseplink = 0;
> char *ssh_host = hostandport;
> const char *port = NULL;
> get_host_and_port(&ssh_host, &port);
> @@ -750,14 +750,26 @@ struct child_process *git_connect(int fd[2], const char
> *url,
> conn->use_shell = 1;
> putty = 0;
> } else {
> + const char *base;
> + char *ssh_dup;
> +
> ssh = getenv("GIT_SSH");
> if (!ssh)
> ssh = "ssh";
> - putty = !!strcasestr(ssh, "plink");
> +
> + ssh_dup = xstrdup(ssh);
> + base = basename(ssh_dup);
> +
> + tortoiseplink = !strcasecmp(base,
> "tortoiseplink") ||
> + !strcasecmp(base,
> "tortoiseplink.exe");
> + putty = !strcasecmp(base, "plink") ||
> + !strcasecmp(base, "plink.exe") ||
> tortoiseplink;
> +
> + free(ssh_dup);
> }
>
> argv_array_push(&conn->args, ssh);
> - if (putty && !strcasestr(ssh, "tortoiseplink"))
> + if (tortoiseplink)
> argv_array_push(&conn->args, "-batch");
> if (port) {
> /* P is for PuTTY, p is for OpenSSH */
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
