[BUG] Trailing backslash in history file confuses entries

[BUG] Trailing backslash in history file confuses entries

[Leah Neukirchen]

Hi,

I noticed this weird behavior:

% zsh --version
zsh 5.8 (x86_64-unknown-linux-gnu)
% zsh -f
rhea% SAVEHIST=9000 HISTSIZE=9000 HISTFILE=/tmp/.zsh_history 
rhea% setopt INC_APPEND_HISTORY EXTENDED_HISTORY
rhea% less foo\<RET><C-c>   # this saves a line with final \ to .zsh_history
rhea% echo bar
bar
rhea% <C-c>
% zsh -f
rhea% SAVEHIST=9000 HISTSIZE=9000 HISTFILE=/tmp/.zsh_history
rhea% fc -R   # load history, same happens without zsh -f by default...
rhea% <UP>
rhea% less foo
: 1636497381:0;echo bar
<C-c>
% tail -n2 /tmp/.zsh_history 
: 1636497373:0;less foo\
: 1636497381:0;echo bar

As you can see, zsh parses the trailing backslash as a line
continuation and assumes the two lines were entered at once
(EXTENDED_HISTORY increases the confusion of the user here).
I assume some kind of escaping has been forgotten in this case.

Thanks,
-- 
Leah Neukirchen  <leah@vuxu.org>  https://leahneukirchen.org/

Re: [BUG] Trailing backslash in history file confuses entries

[Oliver Kiddle]

On 9 Nov, Leah Neukirchen wrote:
> rhea% less foo\<RET><C-c>   # this saves a line with final \ to .zsh_history

Thanks for narrowing this down with a clear example.

You can also break this with print -s but this effect of pressing Ctrl-C
on a PS2 prompt is interesting in that I had never really considered it
before.

The history file use of a trailing backslash to indicate continuation
lines has caused problems before, e.g. in 32882 a space was added where
lines end in an even number of backslashes.

The patch below includes a test case that puts problematic strings in
history, writes them and reads them back in. I'd be grateful if anyone
else can think of further cases and history file contents that might
cause issues.

What this test case doesn't encapsulate is requirements in terms of the
on-disk history file format. Are there any requirements here that I
might have missed? It's clear that some care has gone into using
something that it is close to being valid shell input but it isn't for
multi-line commands.

(ksh93 uses doubled null characters which you can't reinput and
Bash reformats multi-line commands to be single-line commands.)

Currently an extra space is added where the full line ends in an even
number of backslashes. The approach taken in this patch is to add a
single space to any line that ends with any number of backslashes
followed by zero or more spaces. It then strips one space when reading
history if the line ends with backslash followed by one or more spaces.
A backslash followed directly by a newline now always indicates a line
break within a command-line. I hope this preserves original lines
without introducing new issues. And compatibility with old history files
is only broken for fairly contrived cases or those that were already
broken (such as that demonstrated by Leah). It has no problems
with my history file.

Oliver

diff --git a/Src/hist.c b/Src/hist.c
index 6ac581fda..af62cf931 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -2615,12 +2615,19 @@ readhistline(int start, char **bufp, int *bufsiz, FILE *in, int *readbytes)
 	    }
 	}
 	else {
+	    int spc;
 	    buf[len - 1] = '\0';
 	    if (len > 1 && buf[len - 2] == '\\') {
 		buf[--len - 1] = '\n';
 		if (!feof(in))
 		    return readhistline(len, bufp, bufsiz, in, readbytes);
 	    }
+
+	    spc = len - 2;
+	    while (spc >= 0 && buf[spc] == ' ')
+		spc--;
+	    if (spc != len - 2 && buf[spc] == '\\')
+		buf[--len - 1] = '\0';
 	}
 	return len;
     }
@@ -2984,7 +2991,7 @@ savehistfile(char *fn, int err, int writeflags)
 
 	ret = 0;
 	for (; he && he->histnum <= xcurhist; he = down_histent(he)) {
-	    int count_backslashes = 0;
+	    int end_backslashes = 0;
 
 	    if ((writeflags & HFILE_SKIPDUPS && he->node.flags & HIST_DUP)
 	     || (writeflags & HFILE_SKIPFOREIGN && he->node.flags & HIST_FOREIGN)
@@ -3017,18 +3024,14 @@ savehistfile(char *fn, int err, int writeflags)
 		if (*t == '\n')
 		    if ((ret = fputc('\\', out)) < 0)
 			break;
-		if (*t == '\\')
-		    count_backslashes++;
-		else
-		    count_backslashes = 0;
+		end_backslashes = (*t == '\\' || (end_backslashes && *t == ' '));
 		if ((ret = fputc(*t, out)) < 0)
 		    break;
 	    }
 	    if (ret < 0)
 	    	break;
-	    if (count_backslashes && (count_backslashes % 2 == 0))
-		if ((ret = fputc(' ', out)) < 0)
-		    break;
+	    if (end_backslashes)
+		ret = fputc(' ', out);
 	    if (ret < 0 || (ret = fputc('\n', out)) < 0)
 		break;
 	}
diff --git a/Test/W01history.ztst b/Test/W01history.ztst
index 0b2f60d1e..1d3f3cf6f 100644
--- a/Test/W01history.ztst
+++ b/Test/W01history.ztst
@@ -88,3 +88,25 @@ F:Check that a history bug introduced by workers/34160 is working again.
 0:Modifier :P
 >/my/path/for/testing
 >/my/path/for/testing
+
+ $ZTST_testdir/../Src/zsh -fgis <<<'
+ SAVEHIST=7
+ print -rs "one\\"
+ print -rs "two\\\\"
+ print -rs "three\\\\\\"
+ print -rs "four\\\\\\\\"
+ print -rs "five\\\\\\\\\\"
+ print -s  "while false\ndo\ntrue\\\\\n && break\ndone"
+ print -s  "echo one\\\\\ntwo"
+ fc -W hist
+ fc -p -R hist
+ fc -l
+ rm hist' 2>/dev/null
+0:Lines ending in backslash saved and restored to history
+>    1  one\
+>    2  two\\
+>    3  three\\\
+>    4  four\\\\
+>    5  five\\\\\
+>    6  while false\ndo\ntrue\\n && break\ndone
+>    7  echo one\\ntwo