Re: [PATCH 2/2] connect: improve check for plink to reduce false positives
Hi Brian, On 2015-04-24 01:14, brian m. carlson wrote: > On Thu, Apr 23, 2015 at 11:53:04AM -0400, Jeff King wrote: > >> If I were writing from scratch, I would probably keep things as tight as >> possible, like: >> >> const char *base = basename(ssh); >> plink = !strcasecmp(base, "plink") || >> !strcasecmp(base, "plink.exe"); >> tplink = !strcasecmp(base, "tortoiseplink") || >>!strcasecmp(base, "tortoiseplink.exe")); >> >> but maybe that is too tight at this point in time; we don't really know >> what's out there and working (or maybe _we_ do, but _I_ do not :) ). >> >> At any rate, brian's patch only looks for a dir-separator anywhere, not >> the actual basename. So: >> >> /path/to/plink/ssh >> >> would match, and I'm not sure if that's a good thing or not. > > This is true. In hindsight, I think it's probably better to be a bit > stricter, so I'll reroll to use the stricter format. Thank you so much! Dscho -- 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 2/2] connect: improve check for plink to reduce false positives
Hi Peff, On 2015-04-23 17:53, Jeff King wrote: > What about "plink-0.83" that was mentioned earlier in the thread? I was working a lot with Java projects where the base name is often considered to be the part before a version number that is encoded into the file name, so I think it would be a good idea here, too. Essentially, it would be the regex ^(.*[/\\])?plink(-.*|\.exe)$ ... and maybe we should actually do exactly this, use a regex? I know that many developers try to stay away from them, but if you can read them, they are an efficient encoding of expectations. Ciao, Dscho -- 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 2/2] connect: improve check for plink to reduce false positives
On Thu, Apr 23, 2015 at 11:53:04AM -0400, Jeff King wrote: > On Thu, Apr 23, 2015 at 08:50:17AM +0200, Johannes Schindelin wrote: > > > > + tortoiseplink = tplink == ssh || > > > + (tplink && is_dir_sep(tplink[-1])); > > > > Maybe have a helper function here? Something like > > `basename_matches(const char *path, const char *basename, int > > ignore_case)`? That would be easier to read (I have to admit that I > > had to wrap my head around the logic to ensure that tplink[-1] is > > valid; It is, but it requires more brain cycles to verify than I would > > like). > > Yeah, I had a similar thought when reading the patch. I was questioning whether a comment would have been helpful. Apparently the answer was yes. > If I were writing from scratch, I would probably keep things as tight as > possible, like: > > const char *base = basename(ssh); > plink = !strcasecmp(base, "plink") || > !strcasecmp(base, "plink.exe"); > tplink = !strcasecmp(base, "tortoiseplink") || >!strcasecmp(base, "tortoiseplink.exe")); > > but maybe that is too tight at this point in time; we don't really know > what's out there and working (or maybe _we_ do, but _I_ do not :) ). > > At any rate, brian's patch only looks for a dir-separator anywhere, not > the actual basename. So: > > /path/to/plink/ssh > > would match, and I'm not sure if that's a good thing or not. This is true. In hindsight, I think it's probably better to be a bit stricter, so I'll reroll to use the stricter format. -- 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 2/2] connect: improve check for plink to reduce false positives
On Thu, Apr 23, 2015 at 08:50:17AM +0200, Johannes Schindelin wrote: > > + tortoiseplink = tplink == ssh || > > + (tplink && is_dir_sep(tplink[-1])); > > Maybe have a helper function here? Something like > `basename_matches(const char *path, const char *basename, int > ignore_case)`? That would be easier to read (I have to admit that I > had to wrap my head around the logic to ensure that tplink[-1] is > valid; It is, but it requires more brain cycles to verify than I would > like). Yeah, I had a similar thought when reading the patch. > Also, I am really hesitant to just test the start of the basename; I > would rather have an entire basename match so that something like > "PLinKoeln" would not match. (And of course, for Windows I would want > that hypothetical `basename_matches()` function to allow basenames to > end in `.exe` automatically). What about "plink-0.83" that was mentioned earlier in the thread? I think that is the reason brian's patch stuck to matching the start and not the end. But I have no idea if that is actually a real thing, or just a hypothetical. If I were writing from scratch, I would probably keep things as tight as possible, like: const char *base = basename(ssh); plink = !strcasecmp(base, "plink") || !strcasecmp(base, "plink.exe"); tplink = !strcasecmp(base, "tortoiseplink") || !strcasecmp(base, "tortoiseplink.exe")); but maybe that is too tight at this point in time; we don't really know what's out there and working (or maybe _we_ do, but _I_ do not :) ). At any rate, brian's patch only looks for a dir-separator anywhere, not the actual basename. So: /path/to/plink/ssh would match, and I'm not sure if that's a good thing or not. So yet another variant is to use basename(), and then just check that the basename starts with "plink" (to catch "plink.exe", "plink-0.83", etc). That avoids cruft in the intermediate path, and unless your actual binary is named PlinKoeln, it will not false positive on the example you gave. -Peff -- 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 2/2] connect: improve check for plink to reduce false positives
Hi Brian, On 2015-04-23 02:06, brian m. carlson wrote: > + tortoiseplink = tplink == ssh || > + (tplink && is_dir_sep(tplink[-1])); Maybe have a helper function here? Something like `basename_matches(const char *path, const char *basename, int ignore_case)`? That would be easier to read (I have to admit that I had to wrap my head around the logic to ensure that tplink[-1] is valid; It is, but it requires more brain cycles to verify than I would like). Also, I am really hesitant to just test the start of the basename; I would rather have an entire basename match so that something like "PLinKoeln" would not match. (And of course, for Windows I would want that hypothetical `basename_matches()` function to allow basenames to end in `.exe` automatically). In contrast to Torsten, I am not so concerned about `myplink` scripts: that only affects power users who can easily add the `-batch` into the script, where it actually belongs. Ciao, Dscho -- 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
[PATCH 2/2] connect: improve check for plink to reduce false positives
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" only at the beginning of the string or immediately after
a directory separator.
Signed-off-by: brian m. carlson
---
This is essentially just a deindentation.
connect.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/connect.c b/connect.c
index 749a07b..bae76a2 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,23 @@ struct child_process *git_connect(int fd[2], const char
*url,
conn->use_shell = 1;
putty = 0;
} else {
+ char *plink, *tplink;
+
ssh = getenv("GIT_SSH");
if (!ssh)
ssh = "ssh";
- putty = !!strcasestr(ssh, "plink");
+
+ plink = strcasestr(ssh, "plink");
+ tplink = strcasestr(ssh, "tortoiseplink");
+
+ tortoiseplink = tplink == ssh ||
+ (tplink && is_dir_sep(tplink[-1]));
+ putty = tortoiseplink || plink == ssh ||
+ (plink && is_dir_sep(plink[-1]));
}
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 */
--
2.3.5
--
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
