[v2] exec: Stricter pathopt parsing

Commit Message

Herbert Xu May 16, 2018, 5:40 a.m. UTC

On Tue, May 15, 2018 at 11:56:57PM +0200, Jilles Tjoelker wrote:
> 
> Some examples/testcases would make this clearer.

I wanted to keep this undocumented because it's nonstandard and
we may have to change this later.

> Note that this also affects CDPATH (good) and MAILPATH (breaks its
> undocumented feature to replace the "you have mail" text, which seems
> unimportant).

Thanks for the heads up.  I have updated the patch to retain
existing behaviour for MAILPATH and disable % completely for
CDPATH.

---8<---
This patch changes the parsing of pathopt.  First of all only
%builtin and %func (with arbitrary suffixes) will be recognised.
Any other pathopt will be treated as a normal directory.

Furthermore, pathopt can now be specified before the directory,
rather than after it.  In fact, a future version may remove support
for pathopt suffixes.

Wherever the pathopt is placed, an optional % may be placed after
it to terminate the pathopt.

This change is being made so that it is less likely that a genuine
directory containing a % sign is parsed as a pathopt.

Users of padvance outside of exec.c have also been modified:

1) cd(1) will always treat % characters as part of the path.
2) chkmail will continue to accept arbitrary pathopt.
3) find_dot_file will ignore the %builtin pathopt instead of trying
to do a stat in the accompanying directory (which is usually the
current directory).

The patch also removes the clearcmdentry optimisation where we
attempt to only partially flush the table where possible.

Signed-off-by: Herbert Xu <[e-mail]>

Patch

diff --git a/src/cd.c b/src/cd.c
index 610a4fa..b6742af 100644
--- a/src/cd.c
+++ b/src/cd.c
@@ -128,7 +128,7 @@  dotdot:
 	if (!*dest)
 		dest = ".";
 	path = bltinlookup("CDPATH");
-	while (p = path, (len = padvance(&path, dest)) >= 0) {
+	while (p = path, (len = padvance_magic(&path, dest, 0)) >= 0) {
 		c = *p;
 		p = stalloc(len);
 
diff --git a/src/exec.c b/src/exec.c
index 04ee2ba..8948754 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -92,7 +92,7 @@  STATIC int builtinloc = -1;		/* index in path of %builtin, or -1 */
 
 STATIC void tryexec(char *, char **, char **);
 STATIC void printentry(struct tblentry *);
-STATIC void clearcmdentry(int);
+STATIC void clearcmdentry(void);
 STATIC struct tblentry *cmdlookup(const char *, int);
 STATIC void delete_cmd_entry(void);
 STATIC void addcmdentry(char *, struct cmdentry *);
@@ -168,7 +168,27 @@  repeat:
 	}
 }
 
+static const char *legal_pathopt(const char *opt, const char *term, int magic)
+{
+	switch (magic) {
+	case 0:
+		opt = NULL;
+		break;
 
+	case 1:
+		opt = prefix(opt, "builtin") ?: prefix(opt, "func");
+		break;
+
+	default:
+		opt += strcspn(opt, term);
+		break;
+	}
+
+	if (opt && *opt == '%')
+		opt++;
+
+	return opt;
+}
 
 /*
  * Do a path search.  The variable path (passed by reference) should be
@@ -178,39 +198,64 @@  repeat:
  * a percent sign) appears in the path entry then the global variable
  * pathopt will be set to point to it; otherwise pathopt will be set to
  * NULL.
+ *
+ * If magic is 0 then pathopt recognition will be disabled.  If magic is
+ * 1 we shall recognise %builtin/%func.  Otherwise we shall accept any
+ * pathopt.
  */
 
 const char *pathopt;
 
-int padvance(const char **path, const char *name)
+int padvance_magic(const char **path, const char *name, int magic)
 {
+	const char *term = "%:";
+	const char *lpathopt;
 	const char *p;
 	char *q;
 	const char *start;
+	size_t qlen;
 	size_t len;
 
 	if (*path == NULL)
 		return -1;
+
+	lpathopt = NULL;
 	start = *path;
-	for (p = start ; *p && *p != ':' && *p != '%' ; p++);
-	len = p - start + strlen(name) + 2;	/* "2" is for '/' and '\0' */
-	q = growstackto(len);
-	if (p != start) {
-		memcpy(q, start, p - start);
-		q += p - start;
-		*q++ = '/';
+
+	if (*start == '%' && (p = legal_pathopt(start + 1, term, magic))) {
+		lpathopt = start + 1;
+		start = p;
+		term = ":";
 	}
-	strcpy(q, name);
-	pathopt = NULL;
+
+	len = strcspn(start, term);
+	p = start + len;
+
 	if (*p == '%') {
-		pathopt = ++p;
-		while (*p && *p != ':')  p++;
+		size_t extra = strchrnul(p, ':') - p;
+
+		if (legal_pathopt(p + 1, term, magic))
+			lpathopt = p + 1;
+		else
+			len += extra;
+
+		p += extra;
+	}
+
+	pathopt = lpathopt;
+	*path = *p == ':' ? p + 1 : NULL;
+
+	/* "2" is for '/' and '\0' */
+	qlen = len + strlen(name) + 2;
+	q = growstackto(qlen);
+
+	if (likely(len)) {
+		q = mempcpy(q, start, len);
+		*q++ = '/';
 	}
-	if (*p == ':')
-		*path = p + 1;
-	else
-		*path = NULL;
-	return len;
+	strcpy(q, name);
+
+	return qlen;
 }
 
 
@@ -228,7 +273,7 @@  hashcmd(int argc, char **argv)
 	char *name;
 
 	while ((c = nextopt("r")) != '\0') {
-		clearcmdentry(0);
+		clearcmdentry();
 		return 0;
 	}
 	if (*argptr == NULL) {
@@ -363,15 +408,16 @@  find_command(char *name, struct cmdentry *entry, int act, const char *path)
 	idx = -1;
 loop:
 	while ((len = padvance(&path, name)) >= 0) {
+		const char *lpathopt = pathopt;
+
 		fullname = stackblock();
 		idx++;
-		if (pathopt) {
-			if (prefix(pathopt, "builtin")) {
+		if (lpathopt) {
+			if (*lpathopt == 'b') {
 				if (bcmd)
 					goto builtin_success;
 				continue;
-			} else if (!(act & DO_NOFUNC) &&
-				   prefix(pathopt, "func")) {
+			} else if (!(act & DO_NOFUNC)) {
 				/* handled below */
 			} else {
 				/* ignore unimplemented options */
@@ -397,7 +443,7 @@  loop:
 		e = EACCES;	/* if we fail, this will be the error */
 		if (!S_ISREG(statb.st_mode))
 			continue;
-		if (pathopt) {		/* this is a %func directory */
+		if (lpathopt) {		/* this is a %func directory */
 			stalloc(len);
 			readcmdfile(fullname);
 			if ((cmdp = cmdlookup(name, 0)) == NULL ||
@@ -515,39 +561,26 @@  hashcd(void)
 void
 changepath(const char *newval)
 {
-	const char *old, *new;
+	const char *new;
 	int idx;
-	int firstchange;
 	int bltin;
 
-	old = pathval();
 	new = newval;
-	firstchange = 9999;	/* assume no change */
 	idx = 0;
 	bltin = -1;
 	for (;;) {
-		if (*old != *new) {
-			firstchange = idx;
-			if ((*old == '\0' && *new == ':')
-			 || (*old == ':' && *new == '\0'))
-				firstchange++;
-			old = new;	/* ignore subsequent differences */
-		}
-		if (*new == '\0')
-			break;
-		if (*new == '%' && bltin < 0 && prefix(new + 1, "builtin"))
+		if (*new == '%' && prefix(new + 1, "builtin")) {
 			bltin = idx;
-		if (*new == ':') {
-			idx++;
+			break;
 		}
-		new++, old++;
+		new = strchr(new, ':');
+		if (!new)
+			break;
+		idx++;
+		new++;
 	}
-	if (builtinloc < 0 && bltin >= 0)
-		builtinloc = bltin;		/* zap builtins */
-	if (builtinloc >= 0 && bltin < 0)
-		firstchange = 0;
-	clearcmdentry(firstchange);
 	builtinloc = bltin;
+	clearcmdentry();
 }
 
 
@@ -557,7 +590,7 @@  changepath(const char *newval)
  */
 
 STATIC void
-clearcmdentry(int firstchange)
+clearcmdentry(void)
 {
 	struct tblentry **tblp;
 	struct tblentry **pp;
@@ -567,10 +600,8 @@  clearcmdentry(int firstchange)
 	for (tblp = cmdtable ; tblp < &cmdtable[CMDTABLESIZE] ; tblp++) {
 		pp = tblp;
 		while ((cmdp = *pp) != NULL) {
-			if ((cmdp->cmdtype == CMDNORMAL &&
-			     cmdp->param.index >= firstchange)
-			 || (cmdp->cmdtype == CMDBUILTIN &&
-			     builtinloc >= firstchange)) {
+			if (cmdp->cmdtype == CMDNORMAL ||
+			    (cmdp->cmdtype == CMDBUILTIN && builtinloc > 0)) {
 				*pp = cmdp->next;
 				ckfree(cmdp);
 			} else {
diff --git a/src/exec.h b/src/exec.h
index f0c5a28..9dbd5db 100644
--- a/src/exec.h
+++ b/src/exec.h
@@ -64,7 +64,7 @@  extern const char *pathopt;	/* set by padvance */
 
 void shellexec(char **, const char *, int)
     __attribute__((__noreturn__));
-int padvance(const char **, const char *);
+int padvance_magic(const char **path, const char *name, int magic);
 int hashcmd(int, char **);
 void find_command(char *, struct cmdentry *, int, const char *);
 struct builtincmd *find_builtin(const char *);
@@ -77,3 +77,8 @@  void defun(union node *);
 void unsetfunc(const char *);
 int typecmd(int, char **);
 int commandcmd(int, char **);
+
+static inline int padvance(const char **path, const char *name)
+{
+	return padvance_magic(path, name, 1);
+}
diff --git a/src/mail.c b/src/mail.c
index 7f9e49d..8eacb2d 100644
--- a/src/mail.c
+++ b/src/mail.c
@@ -79,7 +79,7 @@  chkmail(void)
 	for (mtp = mailtime; mtp < mailtime + MAXMBOXES; mtp++) {
 		int len;
 
-		len = padvance(&mpath, nullstr);
+		len = padvance_magic(&mpath, nullstr, 2);
 		if (!len)
 			break;
 		p = stackblock();
diff --git a/src/main.c b/src/main.c
index c87fbd7..e8e4256 100644
--- a/src/main.c
+++ b/src/main.c
@@ -300,7 +300,8 @@  find_dot_file(char *basename)
 
 	while ((len = padvance(&path, basename)) >= 0) {
 		fullname = stackblock();
-		if ((stat(fullname, &statb) == 0) && S_ISREG(statb.st_mode)) {
+		if ((!pathopt || *pathopt == 'f') &&
+		    !stat(fullname, &statb) && S_ISREG(statb.st_mode)) {
 			/* This will be freed by the caller. */
 			return stalloc(len);
 		}