On Thu, Aug 25, 2011 at 01:59:29AM +0400, Loganaden Velvindron wrote:
> Can you try the attached diff ?
> Index: src/usr.bin/mg/dired.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mg/dired.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 dired.c
> --- src/usr.bin/mg/dired.c 23 Jan 2011 00:45:03 -0000 1.48
> +++ src/usr.bin/mg/dired.c 24 Aug 2011 21:46:05 -0000
> @@ -627,7 +630,20 @@ dired_(char *dname)
> if (bclear(bp) != TRUE)
> return (NULL);
> bp->b_flag |= BFREADONLY;
> +
> + (void) strlcpy(ini, dname, sizeof(ini));
> + while (strpbrk(&ini[pos], meta) != NULL) {
> + pos = strpbrk(&ini[pos], meta) - &ini[0];
> + (void) strlcpy(tmp, &ini[pos], sizeof(tmp));
> + ini[pos] = '\\';
> + ini[pos+1] = '\0';
> + (void) strlcat(&ini[pos], tmp, sizeof(ini));
There's a potential buffer overrun here. You should pass the buffer
size (sizeof(ini)-pos) to strlcat. You also need to check for
insufficient buffer space (as may occur in a pathname containing 128
parentheses) and fail gracefully if it occurs ("Path too long" as
below).
> + pos += 2;
> + }
> + orig = dname;
> + dname = &ini[0];
> ret = snprintf(line, sizeof(line), "ls -al %s", dname);
> + dname = orig;
> if (ret < 0 || ret >= sizeof(line)) {
> ewprintf("Path too long");
> return (NULL);
I tried the diff and (other than the problem mentioned above) it works
as expected on amd64/4.8 and loongson/current for paths containing
parentheses and spaces. However, it fails on paths with single
quotes, double quotes, astrisks, other globbing characters, etc.
Of course, these characters can be added to the 'meta' string from
your patch ensuring that they also get escaped. I wonder, though, if
it might be less error-prone to just wrap the directory name in single
quotes (as in the patch I included in the PR) and just escape single
quotes in the path name. Any instance of ' in the original string
would have to be replaced with '\'' but we would be guaranteed that
all other special characters are taken care of.
I've attached a patch below. Let me know what you think.
Cheers,
-Nima
--
The pudding is in the proof.
Index: dired.c
===================================================================
RCS file: /var/cvs/src/usr.bin/mg/dired.c,v
retrieving revision 1.48
diff -u -r1.48 dired.c
--- dired.c 23 Jan 2011 00:45:03 -0000 1.48
+++ dired.c 29 Aug 2011 20:09:17 -0000
@@ -600,9 +600,14 @@
{
struct buffer *bp;
FILE *dirpipe;
- char line[256];
- int len, ret, counter, warp;
+ char sdname[256], line[256];
+ int len, dpos, sdpos, ret, counter, warp;
+ char *cptr, *qesc;
+ cptr = NULL;
+ qesc = "'\\''";
counter = 0;
+ dpos = 0;
+ sdpos = 0;
warp = 0;
if ((fopen(dname,"r")) == NULL) {
@@ -627,7 +632,24 @@
if (bclear(bp) != TRUE)
return (NULL);
bp->b_flag |= BFREADONLY;
- ret = snprintf(line, sizeof(line), "ls -al %s", dname);
+
+ while ((cptr = strchr(&dname[dpos], '\'')) != NULL) {
+ if (strlen(qesc) + cptr-&dname[dpos] >= sizeof(sdname)-sdpos) {
+ break; /* not enough room left in sdname */
+ }
+ memcpy(&sdname[sdpos], &dname[dpos], cptr - &dname[dpos]);
+ sdpos += cptr - &dname[dpos];
+ memcpy(&sdname[sdpos], qesc, strlen(qesc));
+ sdpos += strlen(qesc);
+ dpos += cptr - &dname[dpos] + 1;
+ }
+ if (cptr != NULL || sizeof(sdname) <= sdpos) {
+ ewprintf("Path too long");
+ return (NULL);
+ }
+ strlcpy(&sdname[sdpos], &dname[dpos], sizeof(sdname)-sdpos);
+
+ ret = snprintf(line, sizeof(line), "ls -al '%s'", sdname);
if (ret < 0 || ret >= sizeof(line)) {
ewprintf("Path too long");
return (NULL);
