On Sat, Aug 27, 2011 at 08:51:08PM +0300, Henri Kemppainen wrote:
> > The warpdot() has at least one issue. It leads to
> > segfaults if you try to open a directory like (BCD).
>
> [.. diff ..]
> > ed
> >
> > mg doesn't segfault.
>
> Your fix is wrong, and only works by chance. If you craft
> a directory with enough spaces in its name, it'll segfault
> again. And you cannot fix this in warpdot, it's the wrong
> place. The bug is in dired_(), which should abort if the
> command fails.
>
The proper fix is to escape shell metacharacters.
This diff works with all kind of crazy filenames like.
XXX(){}[]'"\;<>|?!$^&~LOL
(AAAA(BBBB(CCCC)DDDD)EEEE
(!^&*~"xxxx
I've looked at the latest diff you've posted on tech@.
The funmap() keybindings are relevant.
There's already strspn(), why write your own
substitute for warpdot() ?
You're also returning by supplying the function inside.
This makes the code less readable.
Lastly, the changes you made in dired_() seem
overcomplicated to me.
You're iterating twice for lforw() and d_warpdot().
In the while() loop, we can already locate the .. and
we just need to lforw() there using the warp variable.
This seems redundant to me.
Here's the diff + my fix for the PR opened by Nima.
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 28 Aug 2011 11:00:05 -0000
@@ -36,6 +36,11 @@ static int d_rename(int, int);
static int d_shell_command(int, int);
static int d_create_directory(int, int);
static int d_makename(struct line *, char *, size_t);
+static int d_warpdot(char *);
+static int d_forwpage(int, int);
+static int d_backpage(int, int);
+static int d_forwline(int, int);
+static int d_backline(int, int);
static void reaper(int);
extern struct keymap_s helpmap, cXmap, metamap;
@@ -57,15 +62,15 @@ static PF dirednul[] = {
static PF diredcl[] = {
reposition, /* ^L */
d_findfile, /* ^M */
- forwline, /* ^N */
+ d_forwline, /* ^N */
rescan, /* ^O */
- backline, /* ^P */
+ d_backline, /* ^P */
rescan, /* ^Q */
backisearch, /* ^R */
forwisearch, /* ^S */
rescan, /* ^T */
universal_argument, /* ^U */
- forwpage, /* ^V */
+ d_forwpage, /* ^V */
rescan, /* ^W */
NULL /* ^X */
};
@@ -77,7 +82,7 @@ static PF diredcz[] = {
rescan, /* ^] */
rescan, /* ^^ */
rescan, /* ^_ */
- forwline, /* SP */
+ d_forwline, /* SP */
d_shell_command, /* ! */
rescan, /* " */
rescan, /* # */
@@ -99,9 +104,9 @@ static PF diredc[] = {
};
static PF diredn[] = {
- forwline, /* n */
+ d_forwline, /* n */
d_ffotherwindow, /* o */
- backline, /* p */
+ d_backline, /* p */
rescan, /* q */
d_rename, /* r */
rescan, /* s */
@@ -116,13 +121,32 @@ static PF direddl[] = {
d_undelbak /* del */
};
+static PF diredbp[] = {
+ d_backpage /* v */
+};
+
+static PF dirednull[] = {
+ NULL
+};
+
#ifndef DIRED_XMAPS
#define NDIRED_XMAPS 0 /* number of extra map sections */
#endif /* DIRED_XMAPS */
-static struct KEYMAPE (6 + NDIRED_XMAPS + IMAPEXT) diredmap = {
- 6 + NDIRED_XMAPS,
- 6 + NDIRED_XMAPS + IMAPEXT,
+static struct KEYMAPE (1 + IMAPEXT) d_backpagemap = {
+ 1,
+ 1 + IMAPEXT,
+ rescan,
+ {
+ {
+ 'v', 'v', diredbp, NULL
+ }
+ }
+};
+
+static struct KEYMAPE (7 + NDIRED_XMAPS + IMAPEXT) diredmap = {
+ 7 + NDIRED_XMAPS,
+ 7 + NDIRED_XMAPS + IMAPEXT,
rescan,
{
#ifndef NO_HELP
@@ -138,6 +162,10 @@ static struct KEYMAPE (6 + NDIRED_XMAPS
CCHR('L'), CCHR('X'), diredcl, (KEYMAP *) & cXmap
},
{
+ CCHR('['), CCHR('['), dirednull, (KEYMAP *) &
+ d_backpagemap
+ },
+ {
CCHR('Z'), '+', diredcz, (KEYMAP *) & metamap
},
{
@@ -165,8 +193,12 @@ dired_init(void)
funmap_add(d_findfile, "dired-find-file");
funmap_add(d_ffotherwindow, "dired-find-file-other-window");
funmap_add(d_del, "dired-flag-file-deleted");
+ funmap_add(d_forwline, "dired-next-line");
funmap_add(d_otherwindow, "dired-other-window");
+ funmap_add(d_backline, "dired-previous-line");
funmap_add(d_rename, "dired-rename-file");
+ funmap_add(d_backpage, "dired-scroll-down");
+ funmap_add(d_forwpage, "dired-scroll-up");
funmap_add(d_undel, "dired-unflag");
maps_add((KEYMAP *)&diredmap, "dired");
dobindkey(fundamental_map, "dired", "^Xd");
@@ -592,6 +624,83 @@ d_makename(struct line *lp, char *fn, si
return ((lgetc(lp, 2) == 'd') ? TRUE : FALSE);
}
+static int
+d_warpdot(char *l_text)
+{
+ char *track, *anchor;
+ int col = 0;
+ int i = 0;
+ anchor = NULL;
+ track = l_text;
+
+ while (*track != '\0') {
+ if((i = strspn(track, " ")) > 0) {
+ track += i;
+ if (++col == 9) {
+ anchor = track;
+ break;
+ }
+ }
+ track++;
+ }
+ if (anchor == NULL)
+ return (-1);
+ else
+ return (track - l_text);
+}
+
+static int
+d_forwpage(int f, int n)
+{
+ int i;
+
+ forwpage(f | FFRAND, n);
+ if ((i = d_warpdot(curwp->w_dotp->l_text)) == -1)
+ curwp->w_doto = 0;
+ else
+ curwp->w_doto = i;
+ return TRUE;
+}
+
+static int
+d_backpage (int f, int n)
+{
+ int i;
+
+ backpage(f | FFRAND, n);
+ if ((i = d_warpdot(curwp->w_dotp->l_text)) == -1)
+ curwp->w_doto = 0;
+ else
+ curwp->w_doto = i;
+ return TRUE;
+}
+
+static int
+d_forwline (int f, int n)
+{
+ int i;
+
+ forwline(f | FFRAND, n);
+ if ((i = d_warpdot(curwp->w_dotp->l_text)) == -1)
+ curwp->w_doto = 0;
+ else
+ curwp->w_doto = i;
+ return TRUE;
+}
+
+static int
+d_backline (int f, int n)
+{
+ int i;
+
+ backline(f | FFRAND, n);
+ if ((i = d_warpdot(curwp->w_dotp->l_text)) == -1)
+ curwp->w_doto = 0;
+ else
+ curwp->w_doto = i;
+ return TRUE;
+}
+
/*
* XXX dname needs to have enough place to store an additional '/'.
*/
@@ -600,10 +709,13 @@ dired_(char *dname)
{
struct buffer *bp;
FILE *dirpipe;
- char line[256];
- int len, ret, counter, warp;
+ char line[256], tmp[256], ini[256];
+ int len, ret, counter, warp, i, pos;
+ char *meta, *orig, *anchor;
counter = 0;
warp = 0;
+ pos = 0;
+ meta = "(){}[]'\"\\;<>|?!$^&~ ";
if ((fopen(dname,"r")) == NULL) {
if (errno == EACCES)
@@ -627,7 +739,19 @@ dired_(char *dname)
if (bclear(bp) != TRUE)
return (NULL);
bp->b_flag |= BFREADONLY;
+ (void) strlcpy(ini, dname, sizeof(ini));
+ while ((anchor = strpbrk(&ini[pos], meta)) != NULL) {
+ pos = anchor - &ini[0];
+ (void) strlcpy(tmp, &ini[pos], sizeof(tmp));
+ ini[pos] = '\\';
+ ini[pos+1] = '\0';
+ (void) strlcat(&ini[pos], tmp, sizeof(ini));
+ 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);
@@ -649,9 +773,11 @@ dired_(char *dname)
if ((strrchr(line,' ')) != NULL) {
if (strcmp((strrchr(line,' '))," ..") == 0)
warp = counter - 1;
- }
- if ((strrchr(line,' ')) != NULL)
- bp->b_doto = strrchr(line,' ') - line + 1;
+ }
+ if ((i = d_warpdot(line)) == -1)
+ bp->b_doto = 0;
+ else
+ bp->b_doto = i;
if (pclose(dirpipe) == -1) {
ewprintf("Problem closing pipe to ls : %s",
strerror(errno));
> > Here's a snippet from ntpd.c
> > if ((pw = getpwnam(NTPD_USER)) == NULL)
> > errx(1, "unknown user %s", NTPD_USER);
> > When you're designing functions, you should account for
> > error values as well. -1 is used by most code written
> > from scratch in base.
>
> The ntpd quote is totally irrelevant. When designing
> functions, you have to account for errors, if such
> can happen. In library functions, returning an error
> value is often the only possible solution, because the
> library does not know how you want to solve the situation.
>
> In other places, there may or may not be a way to handle
> the error immediately. For a simple application, the
> developer could decide that it's simply not worth it trying
> to recover from a failed allocation; in this case, he handles
> it with a wrapper function which simply makes the program
> quit instantly. In other cases, you have options such as
> "do nothing", in which case the operation does not complete,
> but it doesn't cause a crash either. An example: trying
> to move the dot past the start or end of the buffer in mg
> would result in a no-op, and other functions do not have
> to try and verify that the dot is within the legal range.
>
> In the addline case, there is a way to handle the error:
> don't add a line if you can't allocate it. The error
> is handled in such a way that most of the other functions
> do not have to worry about it.
>
> There are other places where errors are handled "up the
> stream" likewise. If you still think this is wrong, I
> could cook you a 20k line diff that makes every single
> function check that 1) the character we read from stdin
> isn't EOF, 2) curwp is not null, 3) curwp->w_dotp is not null,
> 4) curbp is not null, and so on. By now you should realize
> how ridiculous this is. These errors are (or should be)
> accounted for up the stream so that none of the functions
> that actually have to operate on existing buffer or window
> contents should worry about it.
>
> > Your warpdot() works differently
> > and doesn't quite conform to style. You're assigning
> > a value to a variable without checking if this is correct
> > or not. This style is hard to read according to me.
>
> Bullshit. It's not a matter of style; it's a question
It's a matter of style. The way you're designing the loops
and the functions look needlessly complicated to me, and
it's a bit hard to follow.
> of whether or not an error can happen, and what to do when
> it happens. In this case, if there was an error to handle,
> we could return zero, which will always be a legal value (since
> we are assigning to an int which is zero if the line is empty).
Warpdot() is meant to find the 9th column, if it can't, then something's
wrong. It should return -1 to indicate failure. To be safe, doto
should be set to 0.
This is how a LOT of functions in openbsd are written. Why shouldn't
mg conform to that ? If it's not the case, then it should be fixed.
> If warpdot was a library function whose callers need to react
> to the error in different ways, then we'd have to return an
> error value.
>
> Like I said above, the segfault you got has nothing to do with
> warpdot, and trying to fix it there is wrong.
