From dde8da67694d817cedd2627f469a0f453d14b222 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Tue, 16 Aug 2022 21:25:10 +0100 Subject: [PATCH] Fix history comment character in history expansion (re: 41ee12a5) Reproducer: on an interactive shell with the -H option on, $ v=foo $ print ${#v} does not print anything (but should print "3"). The 'print' line also is not added to the history. This bug was exposed by commit 41ee12a5 which enabled the history comment character by default, setting it to '#' as on bash. When it was disabled by default, this bug was rarely exposed. The problem happens here: src/cmd/ksh93/edit/hexpand.c 203: if(hc[2] && *cp == hc[2]) /* history comment designator, skip rest of line */ 204: { 205: stakputc(*cp++); 206: stakputs(cp); 207: DONE(); 208: } The DONE() macro sets the HIST_ERROR bit flag: src/cmd/ksh93/edit/hexpand.c 45: #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. There is another problem. The documentation I added for history expansion states this bit, which is based on bash's behaviour: 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. The intention was to act like bash. src/cmd/ksh93/edit/hexpand.c: - Split the DONE() macro into DONE and ERROROUT of which only the latter sets the HIST_ERROR bit flag. Change usage accordingly. Thix fixes the first problem. - Don't make these new macros function-style macros (with ()) as they end in a goto, so that's a bit misleading. - Add is_wordboundary() which makes a best-effort attempt to determine if the character following the specified character is considered to start a new word by shell grammar rules. Word boundary characters are whitespace and: |&;()`<> - Only recognise the history comment character if is_wordbounary() returns true for the previous character. This fixes the second problem. Thanks to @jghub for the bug report. Resolves: https://github.com/ksh93/ksh/issues/513 --- NEWS | 7 ++++++ src/cmd/ksh93/edit/hexpand.c | 41 ++++++++++++++++++++++++--------- src/cmd/ksh93/include/version.h | 4 ++-- src/cmd/ksh93/tests/pty.sh | 25 ++++++++++++++++++++ 4 files changed, 64 insertions(+), 13 deletions(-) diff --git a/NEWS b/NEWS index 165496a50..de62d0613 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,13 @@ This documents significant changes in the 1.0 branch of ksh 93u+m. For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0 Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library. +2022-08-16: + +- Fixed an old bug in history expansion (set -H) where any use of the history + comment character caused processing to be aborted as if it were an invalid + history expansion. (On 2022-01-24 the history comment character was set to + '#' instead of being disabled by default, which exposed this bug.) + 2022-08-08: - Release 1.0.2. diff --git a/src/cmd/ksh93/edit/hexpand.c b/src/cmd/ksh93/edit/hexpand.c index 8c421cb36..df0add186 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,16 @@ static char *parse_subst(const char *s, struct subst *sb) return cp; } +/* + * return true if c is a word boundary character, i.e. the + * character following c is considered to start a new word + */ + +static int is_wordboundary(char c) +{ + return isspace(c) || strchr("|&;()`<>",c); +} + /* * history expansion main routine */ @@ -200,11 +211,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 +341,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 +446,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 +507,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 +567,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 +627,7 @@ getsel: (flag & HIST_QUICKSUBST) ? ":s" : "", evp); *cp = c; - DONE(); + ERROROUT; } /* need pointer for strstr() */ @@ -636,7 +655,7 @@ getsel: (flag & HIST_QUICKSUBST) ? ":s" : "", evp); *cp = c; - DONE(); + ERROROUT; } /* loop if g modifier specified */ if(!tempcp || !(flag & HIST_GLOBALSUBST)) diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 7a0265db8..65fc12eea 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -17,8 +17,8 @@ #include #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ -#define SH_RELEASE_SVER "1.0.2" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2022-08-08" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_SVER "1.0.3-alpha" /* semantic version number: https://semver.org */ +#define SH_RELEASE_DATE "2022-08-16" /* must be in this format for $((.sh.version)) */ #define SH_RELEASE_CPYR "(c) 2020-2022 Contributors to ksh " SH_RELEASE_FORK /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */ diff --git a/src/cmd/ksh93/tests/pty.sh b/src/cmd/ksh93/tests/pty.sh index ecdf949e6..7e00fc760 100755 --- a/src/cmd/ksh93/tests/pty.sh +++ b/src/cmd/ksh93/tests/pty.sh @@ -1021,5 +1021,30 @@ u Done u Done u Done ! + +((SHOPT_HISTEXPAND)) && HISTFILE=$tmp/tmp_histfile tst $LINENO <<"!" +L history expansion: history comment character stops line from being processed +# https://github.com/ksh93/ksh/issues/513 + +d 15 +p :test-1: +w set -H +p :test-2: +w true ${#v} !non_existent +r true \$\{#v} !non_existent\r\n$ +r : !non_existent: event not found +w histchars='!^@' +p :test-3: +w true \\@ !non_existent +r true \\@ !non_existent\r\n$ +r : !non_existent: event not found +p :test-4: +w true @ !non_existent +r true @ !non_existent\r\n$ +p :test-5: +w true OK +r true OK\r\n$ +! + # ====== exit $((Errors<125?Errors:125))