[v5] quote arguments in xtrace output

Commit Message

Martijn Dekker March 29, 2018, 8:28 a.m. UTC

The xtrace (set -x) output in dash is a bit of a pain, because arguments
containing whitespace aren't quoted. This can it impossible to tell
which bit belongs to which argument:

$ dash -c 'set -x; printf "%s\n" one "two three" four'
+ printf %s\n one two three four
one
two three
four

Another disadvantage is you cannot simply copy and paste the commands
from this xtrace output to repeat them for debugging purposes.

The attached patch shell-quotes xtrace arguments that contain
non-shell-safe characters or are identical to reserved words.

A parameter is added to single_quote() indicating whether quoting should
be unconditional (0) or conditional upon the string containing
characters that are not shell-safe (1). This leaves the unconditional
quoting in the output of commands like 'trap' and 'set' unchanged while
avoiding excessive quoting in xtrace output.

Shell-safe characters are defined as this set:
0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz%+,./:@_^-

Quoting of assignments needs to be handled specially; in order for the
output to be suitable for re-entry into the shell, only the part after
the "=" must be quoted. For this purpose, eprintlist() was split in into
two functions, eprintvarlist() to print the variable assignments and
eprintarglist() to print the rest.

I've had this patch in use for a year and found it to work reliably, so
I hope it can be considered. The only differences with v4, sent on 18
March 2017, is that I fixed a line with trailing whitespace, and that it
should now be good for Patchwork.

Thanks,

- M.

Comments

Herbert Xu April 19, 2018, 10:14 a.m. UTC | #1

Martijn Dekker <[e-mail]> wrote:
> 
> The xtrace (set -x) output in dash is a bit of a pain, because arguments
> containing whitespace aren't quoted. This can it impossible to tell
> which bit belongs to which argument:

Sorry, but one of the key goals of dash is to be as small as
possible.  So that means no features unless absolutely necessary.

As such I cannot accept this patch as it is.

Thanks,

Martijn Dekker April 19, 2018, 12:47 p.m. UTC | #2

Op 19-04-18 om 12:14 schreef Herbert Xu:
> Sorry, but one of the key goals of dash is to be as small as
> possible.  So that means no features unless absolutely necessary.
> 
> As such I cannot accept this patch as it is.

Would you accept it if it were a configure option, disabled by default 
(like linking with libedit)?

- Martijn

--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Herbert Xu April 27, 2018, 6:43 a.m. UTC | #3

On Thu, Apr 19, 2018 at 02:47:42PM +0200, Martijn Dekker wrote:
> 
> Would you accept it if it were a configure option, disabled by default (like
> linking with libedit)?

Sorry, I don't think this particular feature would fit well with
a configuration option.

Cheers,

Patch

diff --git a/src/alias.c b/src/alias.c
index daeacbb..7a88b44 100644
--- a/src/alias.c
+++ b/src/alias.c
@@ -197,7 +197,7 @@  freealias(struct alias *ap) {
 
 void
 printalias(const struct alias *ap) {
-	out1fmt("%s=%s\n", ap->name, single_quote(ap->val));
+	out1fmt("%s=%s\n", ap->name, single_quote(ap->val, 0));
 }
 
 STATIC struct alias **
diff --git a/src/eval.c b/src/eval.c
index 7498f9d..aba305e 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -95,7 +95,8 @@  STATIC int evalcommand(union node *, int);
 STATIC int evalbltin(const struct builtincmd *, int, char **, int);
 STATIC int evalfun(struct funcnode *, int, char **, int);
 STATIC void prehash(union node *);
-STATIC int eprintlist(struct output *, struct strlist *, int);
+STATIC int eprintvarlist(struct output *, struct strlist *, int);
+STATIC void eprintarglist(struct output *, struct strlist *, int);
 STATIC int bltincmd(int, char **);
 
 
@@ -786,8 +787,8 @@  evalcommand(union node *cmd, int flags)
 		out = &preverrout;
 		outstr(expandstr(ps4val()), out);
 		sep = 0;
-		sep = eprintlist(out, varlist.list, sep);
-		eprintlist(out, arglist.list, sep);
+		sep = eprintvarlist(out, varlist.list, sep);
+		eprintarglist(out, arglist.list, sep);
 		outcslow('\n', out);
 #ifdef FLUSHERR
 		flushout(out);
@@ -1107,16 +1108,35 @@  execcmd(int argc, char **argv)
 
 
 STATIC int
-eprintlist(struct output *out, struct strlist *sp, int sep)
+eprintvarlist(struct output *out, struct strlist *sp, int sep)
 {
 	while (sp) {
 		const char *p;
+		int i;
 
-		p = " %s" + (1 - sep);
+		if (sep)
+			outfmt(out, " ");
 		sep |= 1;
-		outfmt(out, p, sp->text);
+		i = 0;
+		while (sp->text[i] != '=' && sp->text[i] != '\0')
+			outfmt(out, "%c", sp->text[i++]);
+		if (sp->text[i] == '=')
+			outfmt(out, "=%s", single_quote(sp->text+i+1, 1));
 		sp = sp->next;
 	}
 
 	return sep;
 }
+
+STATIC void
+eprintarglist(struct output *out, struct strlist *sp, int sep)
+{
+	while (sp) {
+		const char *p;
+
+		p = " %s" + (1 - sep);
+		sep |= 1;
+		outfmt(out, p, single_quote(sp->text, 1));
+		sp = sp->next;
+	}
+}
diff --git a/src/mystring.c b/src/mystring.c
index de624b8..e13b1a6 100644
--- a/src/mystring.c
+++ b/src/mystring.c
@@ -55,6 +55,7 @@ 
 #include "memalloc.h"
 #include "parser.h"
 #include "system.h"
+#include "token_vars.h"
 
 
 char nullstr[1];		/* zero length string */
@@ -184,13 +185,20 @@  is_number(const char *p)
 
 /*
  * Produce a possibly single quoted string suitable as input to the shell.
- * The return string is allocated on the stack.
+ * If 'conditional' is nonzero, quoting is only done if the string contains
+ * non-shellsafe characters, or is identical to a shell keyword (reserved
+ * word); if it is zero, quoting is always done.
+ * If quoting was done, the return string is allocated on the stack,
+ * otherwise a pointer to the original string is returned.
  */
 
 char *
-single_quote(const char *s) {
+single_quote(const char *s, int conditional) {
 	char *p;
 
+	if (conditional && *s != '\0' && s[strspn(s, SHELLSAFECHARS)] == '\0' && ! findkwd(s))
+		return (char *)s;
+
 	STARTSTACKSTR(p);
 
 	do {
diff --git a/src/mystring.h b/src/mystring.h
index 083ea98..d89dde8 100644
--- a/src/mystring.h
+++ b/src/mystring.h
@@ -54,10 +54,13 @@  intmax_t atomax(const char *, int);
 intmax_t atomax10(const char *);
 int number(const char *);
 int is_number(const char *);
-char *single_quote(const char *);
+char *single_quote(const char *, int);
 char *sstrdup(const char *);
 int pstrcmp(const void *, const void *);
 const char *const *findstring(const char *, const char *const *, size_t);
 
 #define equal(s1, s2)	(strcmp(s1, s2) == 0)
 #define scopy(s1, s2)	((void)strcpy(s2, s1))
+
+/* Characters that don't need quoting before re-entry into the shell */
+#define SHELLSAFECHARS "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz%+,./:@_^-"
diff --git a/src/trap.c b/src/trap.c
index 69eb8ab..a0e4612 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -107,7 +107,7 @@  trapcmd(int argc, char **argv)
 			if (trap[signo] != NULL) {
 				out1fmt(
 					"trap -- %s %s\n",
-					single_quote(trap[signo]),
+					single_quote(trap[signo], 0),
 					signal_names[signo]
 				);
 			}
diff --git a/src/var.c b/src/var.c
index cc6f7f2..414235f 100644
--- a/src/var.c
+++ b/src/var.c
@@ -417,7 +417,7 @@  showvars(const char *prefix, int on, int off)
 		p = strchrnul(*ep, '=');
 		q = nullstr;
 		if (*p)
-			q = single_quote(++p);
+			q = single_quote(++p, 0);
 
 		out1fmt("%s%s%.*s%s\n", prefix, sep, (int)(p - *ep), *ep, q);
 	}