history expansion: history comment character stops line from being processed

[jghub]

v=foo
print ${#v}

does not print anything (but should print "3"). also, the print line does not make it into the history file which looks like a "silent crash" of the ${#v} evaluation or similar, no? same thing with

len=${#v}

(len is empty and the assignment statement does not make it into the history).

is this me or is this a bug?

thanks,
joerg

ps: Version AJM 93u+m/1.1.0-alpha+9067bb3c/MOD 2022-08-06

edit: I now put those two lines in an executable script tmp: that works as
• tmp
• ksh tmp
• source tmp

it only fails when issuing the commands interactively in ksh93u+m. no idea how this can come about, especially the last observation, that source succeeds.

[McDutchie]

I can reproduce this if history expansion (-H, -o histexpand) is active. I cannot reproduce it on 93u+ at all. So it may be that we introduced a bug in history expansion.

[jghub]

I can reproduce this if history expansion (-H, -o histexpand) is active. I cannot reproduce it on 93u+ at all. So it may be that we introduced a bug in history expansion.

yes, I do have set -H in my .kshrc (once upon a time I actually was using ancient csh and old habits die hard ....) and indeed deactivating that restores sane behaviour ... and I confirmed (prior to posting) that it does work in 93u+ (but thought it probably redundant to explicitly mention it. sorry for that).

and of course I do not understand at all how set -H could interfere with ${#parameter} :).

[McDutchie]

The problem was introduced in commit 893df4d.

This commit sets '#' as the default history comment character as on bash; it no longer disables it by default.

Yet, bash does not have this problem with history expansion and the history comment character set to '#'. So this change exposed a pre-existing bug.

To reproduce in 93u+:

$ ENV=/./dev/null /bin/ksh
$ echo ${.sh.version}
Version AJM 93u+ 2012-08-01
$ set -o histexpand
$ histchars='!^#'
$ echo ${#v}
$

[McDutchie]

I never use history expansion because I hate it; it overrides and interferes with regular shell grammar parsing. Since there are people who like and use it, I did do my best to test and fix it, but clearly I've overlooked this bug. All use of the history comment character simply stops the line from being processed at all, e.g.:

$ set +H
$ echo test #comment
test
$ set -H
$ histchars='!^#'
$ echo test #comment
$ histchars='!^@'
$ echo test @comment
$

[McDutchie]

The problem happens here:

ksh/src/cmd/ksh93/edit/hexpand.c

Lines 203 to 208 in 2ee98ca

	if(hc[2] && *cp == hc[2]) /* history comment designator, skip rest of line */
	{
	stakputc(*cp++);
	stakputs(cp);
	DONE();
	}

The DONE macro sets the HIST_ERROR bit flag:

ksh/src/cmd/ksh93/edit/hexpand.c

Line 45 in 2ee98ca

#define DONE() {flag |= HIST_ERROR; cp = 0; stakseek(0); goto done;}

For the history comment character that is clearly wrong, as no error has occurred.

Please test this patch (click to expand, click icon to copy).

diff --git a/src/cmd/ksh93/edit/hexpand.c b/src/cmd/ksh93/edit/hexpand.c
index 8c421cb36..d0a1c2ab2 100644
--- a/src/cmd/ksh93/edit/hexpand.c
+++ b/src/cmd/ksh93/edit/hexpand.c
@@ -42,7 +42,8 @@ NoN(hexpand)
 static char *modifiers = "htrepqxs&";
 static int mod_flags[] = { 0, 0, 0, 0, HIST_PRINT, HIST_QUOTE, HIST_QUOTE|HIST_QUOTE_BR, 0, 0 };

-#define	DONE()	{flag |= HIST_ERROR; cp = 0; stakseek(0); goto done;}
+#define	DONE()		{ cp = 0; stakseek(0); goto done; }
+#define	ERROROUT()	{ flag |= HIST_ERROR; DONE(); }

 struct subst
 {
@@ -322,7 +323,7 @@ getline:
 			*cp = '\0';
 			errormsg(SH_DICT, ERROR_ERROR, "%s: event not found", evp);
 			*cp = c;
-			DONE();
+			ERROROUT();
 		}

 		if(str) /* string search: restore orig. line */
@@ -427,7 +428,7 @@ getline:
 			*cp = '\0';
 			errormsg(SH_DICT, ERROR_ERROR, "%s: bad word specifier", evp);
 			*cp = c;
-			DONE();
+			ERROROUT();
 		}

 		/* no valid word designator after colon, rewind */
@@ -488,7 +489,7 @@ getsel:
 			*cp = '\0';
 			errormsg(SH_DICT, ERROR_ERROR, "%s: bad word specifier", evp);
 			*cp = c;
-			DONE();
+			ERROROUT();
 		}
 		else if(w[1] == -2)	/* skip last word */
 			sfseek(tmp, i, SEEK_SET);
@@ -548,7 +549,7 @@ getsel:
 			else
 			{
 				errormsg(SH_DICT, ERROR_ERROR, "%c: unrecognized history modifier", c);
-				DONE();
+				ERROROUT();
 			}

 			if(c == 'h' || c == 'r') /* head or base */
@@ -608,7 +609,7 @@ getsel:
 						(flag & HIST_QUICKSUBST) ? ":s" : "",
 						evp);
 					*cp = c;
-					DONE();
+					ERROROUT();
 				}

 				/* need pointer for strstr() */
@@ -636,7 +637,7 @@ getsel:
 							(flag & HIST_QUICKSUBST) ? ":s" : "",
 							evp);
 						*cp = c;
-						DONE();
+						ERROROUT();
 					}
 					/* loop if g modifier specified */
 					if(!tempcp || !(flag & HIST_GLOBALSUBST))

[jghub]

I never use history expansion because I hate it; it overrides and interferes with regular shell grammar parsing.

can't argue against hate :).

Since there are people who like and use it, I did do my best to test and fix it, but clearly I've overlooked this bug. All use of

yes, I think the shell should continue to support it. at the very least !!' and !string` are more convenient than canonical reverse history search to quickly recall lengthy commands. at least my finger memory tells me so...

the history comment character simply stops the line from being processed at all, e.g.:

$ set +H
$ echo test #comment
test
$ set -H
$ histchars='!^#'
$ echo test #comment
$ histchars='!^@'
$ echo test @comment
$

[jghub]

The problem happens here:

ksh/src/cmd/ksh93/edit/hexpand.c

Lines 203 to 208 in 2ee98ca

	if(hc[2] && *cp == hc[2]) /* history comment designator, skip rest of line */
	{
	stakputc(*cp++);
	stakputs(cp);
	DONE();
	}

The DONE macro sets the HIST_ERROR bit flag:

ksh/src/cmd/ksh93/edit/hexpand.c

Line 45 in 2ee98ca

#define DONE() {flag |= HIST_ERROR; cp = 0; stakseek(0); goto done;}

For the history comment character that is clearly wrong, as no error has occurred.

Please test this patch (click to expand, click icon to copy).

done, and congrats: that works! thanks for the super-quick fix 👍

[McDutchie]

Thanks for the report!

This fix is probably worth a 1.0.3 point release as the effects are quite serious.

[McDutchie]

There is another problem. The documentation I added for history expansion (based on the tcsh(1) man page) states:

If a word on a command line begins with the history comment character #, history expansion is ignored for the rest of that line.

With an expansion like ${#v}, the word does not begin with # so history expansion should not have parsed that as a comment character. This should only be done if the preceding character is whitespace or if it is at the beginning of the line.

That's how bash acts, too, so we should definitely change to conform with that.

[jghub]

With an expansion like ${#v}, the word does not begin with # so history expansion should not have parsed that as a comment character. This should only be done if the preceding character is whitespace or if it is at the beginning of the line.

whitespace or start of line or after ; ?

[McDutchie]

Good point. & too.

[jghub]

right. back to square no. 1: ultimately, all instances where '#' does not start a word are not comments (there are quite a number of different uses of '#' as part of the syntax listed in the manpage...).

out of curiosity and without any knowledge of the code/architecture, so maybe naive/clueless: does this not ultimately mean only the ksh lexer/tokenizer can reliably decide what a comment is (and throw it away) and what not? and history expansion is not using (the same strategy as) the lexer but parsing the input lines independently and therefore can fail to do it correctly?

[McDutchie]

does this not ultimately mean only the ksh lexer/tokenizer can reliably decide what a comment is (and throw it away) and what not? and history expansion is not using (the same strategy as) the lexer but parsing the input lines independently and therefore can fail to do it correctly?

Exactly. And that is precisely the reason why I dislike history expansion. The fundamental design is flawed, so there will always be corner cases where this incompatibility may bite you. The best we can do is minimise them.

Also note that, when you change the history expansion comment character (by setting the third character of the histchars variable), the shell grammar comment character will not change along with it.

Here's patch version two. It should fix this bug and make the history expansion comment character act like bash with regard to word boundaries. Please test.

Patch version 2

diff --git a/src/cmd/ksh93/edit/hexpand.c b/src/cmd/ksh93/edit/hexpand.c
index 8c421cb36..ac6e5a9db 100644
--- a/src/cmd/ksh93/edit/hexpand.c
+++ b/src/cmd/ksh93/edit/hexpand.c
@@ -42,7 +42,8 @@ NoN(hexpand)
 static char *modifiers = "htrepqxs&";
 static int mod_flags[] = { 0, 0, 0, 0, HIST_PRINT, HIST_QUOTE, HIST_QUOTE|HIST_QUOTE_BR, 0, 0 };

-#define	DONE()	{flag |= HIST_ERROR; cp = 0; stakseek(0); goto done;}
+#define DONE		{ stakseek(0); goto done; }
+#define ERROROUT	{ flag |= HIST_ERROR; DONE; }

 struct subst
 {
@@ -127,6 +128,15 @@ static char *parse_subst(const char *s, struct subst *sb)
 	return cp;
 }

+/*
+ * return true if character is a word boundary for history comment character
+ */
+
+static int is_wordboundary(char c)
+{
+	return isspace(c) || strchr("|&;()`<>",c);
+}
+
 /*
  * history expansion main routine
  */
@@ -200,11 +210,19 @@ int hist_expand(const char *ln, char **xp)
 			continue;
 		}

-		if(hc[2] && *cp == hc[2]) /* history comment designator, skip rest of line */
+		if(hc[2] && *cp == hc[2])
 		{
-			stakputc(*cp++);
-			stakputs(cp);
-			DONE();
+			if(cp == ln || is_wordboundary(cp[-1]))
+			{
+				/* word begins with history comment character; skip rest of line */
+				stakputs(cp);
+				DONE;
+			}
+			else
+			{
+				stakputc(*cp++);
+				continue;
+			}
 		}

 		n = -1;
@@ -322,7 +340,7 @@ getline:
 			*cp = '\0';
 			errormsg(SH_DICT, ERROR_ERROR, "%s: event not found", evp);
 			*cp = c;
-			DONE();
+			ERROROUT;
 		}

 		if(str) /* string search: restore orig. line */
@@ -427,7 +445,7 @@ getline:
 			*cp = '\0';
 			errormsg(SH_DICT, ERROR_ERROR, "%s: bad word specifier", evp);
 			*cp = c;
-			DONE();
+			ERROROUT;
 		}

 		/* no valid word designator after colon, rewind */
@@ -488,7 +506,7 @@ getsel:
 			*cp = '\0';
 			errormsg(SH_DICT, ERROR_ERROR, "%s: bad word specifier", evp);
 			*cp = c;
-			DONE();
+			ERROROUT;
 		}
 		else if(w[1] == -2)	/* skip last word */
 			sfseek(tmp, i, SEEK_SET);
@@ -548,7 +566,7 @@ getsel:
 			else
 			{
 				errormsg(SH_DICT, ERROR_ERROR, "%c: unrecognized history modifier", c);
-				DONE();
+				ERROROUT;
 			}

 			if(c == 'h' || c == 'r') /* head or base */
@@ -608,7 +626,7 @@ getsel:
 						(flag & HIST_QUICKSUBST) ? ":s" : "",
 						evp);
 					*cp = c;
-					DONE();
+					ERROROUT;
 				}

 				/* need pointer for strstr() */
@@ -636,7 +654,7 @@ getsel:
 							(flag & HIST_QUICKSUBST) ? ":s" : "",
 							evp);
 						*cp = c;
-						DONE();
+						ERROROUT;
 					}
 					/* loop if g modifier specified */
 					if(!tempcp || !(flag & HIST_GLOBALSUBST))