[PATCH] Fix an issue where SIGINT leaves readhistfile in inconsistent state

[PATCH] Fix an issue where SIGINT leaves readhistfile in inconsistent state

[Yutian Li]

Dear all,

Thanks for perfecting this wonderful project. I've been using it for
years, and it's my first time trying to contribute back to the
community. So definitely let me know if my patch needs more work. :)

The issue I encountered is, when Zsh receives a SIGINT, it sets
errflag |= ERRFLAG_INT so it will break out of the loop in
readhistfile. But before we entered this loop we have already set
lasthist.fsiz = sb.st_size and lasthist.mtim = sb.st_mtime so it looks
like we have processed the history file up to the end already. And
next time readhistfile is called, it will skip all processing and
assume we have the entire history. This leaves Zsh in an inconsistent
state, where history from different sessions could be lost.

As a quick reproduction, setopt share_history, set HISTFILE
accordingly and open two sessions. In one session, do a bunch of
```
echo a
echo b
echo c
echo d
```
Then in the second session, do
```
<Ctrl-C>
history
```
In the history shown, you'll only see `echo a` but not the rest. The
reason is after Zsh processed the first line, and since Ctrl-C sets
`ERRFLAG_INT`, it will stop processing histories but think it has
processed everything up to the end of the file. So now that history is
basically eaten up. And worse still, if the history file gets
truncated/rewritten/`fc -W` at the end of the session, that history
will be lost forever.

My proposed fix is to just add another flag to tell us if we've been
interrupted from last time. If so, we don't skip processing.

Thanks,
Yutian

diff --git a/Src/hist.c b/Src/hist.c
index dbdc1e4e5..e3950e064 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -216,6 +216,7 @@ static struct histfile_stats {
     char *text;
     time_t stim, mtim;
     off_t fpos, fsiz;
+    int interrupted;
     zlong next_write_ev;
 } lasthist;

@@ -2544,11 +2545,13 @@ readhistfile(char *fn, int err, int readflags)
  sb.st_size == 0)
  return;
     if (readflags & HFILE_FAST) {
- if ((lasthist.fsiz == sb.st_size && lasthist.mtim == sb.st_mtime)
-     || lockhistfile(fn, 0))
+ if (!lasthist.interrupted &&
+     ((lasthist.fsiz == sb.st_size && lasthist.mtim == sb.st_mtime)
+     || lockhistfile(fn, 0)))
      return;
  lasthist.fsiz = sb.st_size;
  lasthist.mtim = sb.st_mtime;
+ lasthist.interrupted = 0;
     } else if ((ret = lockhistfile(fn, 1))) {
  if (ret == 2) {
      zwarn("locking failed for %s: %e: reading anyway", fn, errno);
@@ -2694,8 +2697,10 @@ readhistfile(char *fn, int err, int readflags)
       */
      if (uselex || remeta)
  freeheap();
-     if (errflag & ERRFLAG_INT)
+     if (errflag & ERRFLAG_INT) {
+ lasthist.interrupted=1;
  break;
+     }
  }
  if (start && readflags & HFILE_USE_OPTIONS) {
      zsfree(lasthist.text);

Re: [PATCH] Fix an issue where SIGINT leaves readhistfile in inconsistent state

[Peter Stephenson]

On Sat, 2019-02-16 at 23:40 -0500, Yutian Li wrote:
> The issue I encountered is, when Zsh receives a SIGINT, it sets
> errflag |= ERRFLAG_INT so it will break out of the loop in
> readhistfile. But before we entered this loop we have already set
> lasthist.fsiz = sb.st_size and lasthist.mtim = sb.st_mtime so it looks
> like we have processed the history file up to the end already. And
> next time readhistfile is called, it will skip all processing and
> assume we have the entire history. This leaves Zsh in an inconsistent
> state, where history from different sessions could be lost.
>
> My proposed fix is to just add another flag to tell us if we've been
> interrupted from last time. If so, we don't skip processing.

That seems fine --- it looks like it ought to be safe.  I've committed
it.

Thanks
pws