Bug: Searching through sufficiently large $historywords causes seg fault

Bug: Searching through sufficiently large $historywords causes seg fault

[Marlon Richert]

However, this does not happen when searching through a normal array of
the same size:

% tmp=( {1..99999} )
% : $tmp[(r)foo]
% print -S "$tmp"
% : $historywords[(r)foo]
zsh: segmentation fault

Re: Bug: Searching through sufficiently large $historywords causes seg fault

[Bart Schaefer]

Does PWS's patch from https://zsh.org/workers/51722 make any difference?

Re: Bug: Searching through sufficiently large $historywords causes seg fault

[Bart Schaefer]

On Mon, May 8, 2023 at 10:01 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> Does PWS's patch from https://zsh.org/workers/51722 make any difference?

To attempt to answer my own question:

I don't get the segfault on $historywords[(r)foo] in either case
(without, or with, 51722).  However, after [(r)foo] in the "without"
case I get an immediate segfault attempting $#historywords.

With 51722, $#historywords is incorrect (returns "51" where it should
be about 100,000) and I eventually get a segfault after scrolling back
through the history past the event that was added with "print -S'.

Re: Bug: Searching through sufficiently large $historywords causes seg fault

[Bart Schaefer]

On Tue, May 9, 2023 at 9:53 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> With 51722, $#historywords is incorrect (returns "51" where it should
> be about 100,000) and I eventually get a segfault after scrolling back
> through the history past the event that was added with "print -S'.

Scratch that ... I had not correctly recompiled.  After correctly
recompiling, I can't reproduce the segmentation fault at all with
51722, but the event added with "print -S" is entirely missing from
$historywords even though it is found when scrolling the history.

Re: Bug: Searching through sufficiently large $historywords causes seg fault

[Peter Stephenson]

On Tue, 2023-05-09 at 09:58 -0700, Bart Schaefer wrote:
> On Tue, May 9, 2023 at 9:53 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
> > With 51722, $#historywords is incorrect (returns "51" where it should
> > be about 100,000) and I eventually get a segfault after scrolling back
> > through the history past the event that was added with "print -S'.
> 
> Scratch that ... I had not correctly recompiled.  After correctly
> recompiling, I can't reproduce the segmentation fault at all with
> 51722, but the event added with "print -S" is entirely missing from
> $historywords even though it is found when scrolling the history.

That sounds fine --- it's the division into words, not the line, that's
causing the problems which this avoids.

In this case it's presumably the original word counting, not the
re-spliting on reading a history file, that's causing the underlying
bad numbers.

pws

Re: Bug: Searching through sufficiently large $historywords causes seg fault

[Bart Schaefer]

On Tue, May 9, 2023 at 12:01 PM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> On Tue, 2023-05-09 at 09:58 -0700, Bart Schaefer wrote:
> >
> > Scratch that ... I had not correctly recompiled.  After correctly
> > recompiling, I can't reproduce the segmentation fault at all with
> > 51722, but the event added with "print -S" is entirely missing from
> > $historywords even though it is found when scrolling the history.
>
> That sounds fine --- it's the division into words, not the line, that's
> causing the problems which this avoids.

It's fine that an event is missing from historywords?  Doesn't that
mean that we've just skipped over the problem code rather than figure
out why it's wrong?

Re: Bug: Searching through sufficiently large $historywords causes seg fault

[Peter Stephenson]

> On 10/05/2023 00:05 Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Tue, May 9, 2023 at 12:01 PM Peter Stephenson
> <p.w.stephenson@ntlworld.com> wrote:
> >
> > On Tue, 2023-05-09 at 09:58 -0700, Bart Schaefer wrote:
> > >
> > > Scratch that ... I had not correctly recompiled.  After correctly
> > > recompiling, I can't reproduce the segmentation fault at all with
> > > 51722, but the event added with "print -S" is entirely missing from
> > > $historywords even though it is found when scrolling the history.
> >
> > That sounds fine --- it's the division into words, not the line, that's
> > causing the problems which this avoids.
> 
> It's fine that an event is missing from historywords?  Doesn't that
> mean that we've just skipped over the problem code rather than figure
> out why it's wrong?

The current patch is entirely about safety to cover all the possible
cases where history word numbering is going wrong and preventing crashes.
Completion is particularly sensitive to this as it looks at all words
for all lines, so if it can go wrong, it will.  The patch now seems to
be doing its job.

Tracking down all possible cases where a history word number can be
calculated wrongly from any possible source is another thing entirely.
Possibly some DPUTS() in the earlier code are a good idea, as now we
know it is showing up in error later on trapping it there isn't going
to help any more.

pws

Re: Bug: Searching through sufficiently large $historywords causes seg fault

[Peter Stephenson]

> Tracking down all possible cases where a history word number can be
> calculated wrongly from any possible source is another thing entirely.
> Possibly some DPUTS() in the earlier code are a good idea, as now we
> know it is showing up in error later on trapping it there isn't going
> to help any more.

Here's the only obvious case I can see in normal history management
where you might get a miscalculation, though why is obscure as this
seems only to be connected to skipping a !-history expansion.  IF
this pops up hopefully you'll be able to work out how to reproduce
it.

The history reading mechanism from a file remains a suspect as it's
much more obscure, and is consequently harder to instrument usefully.

I'll commit the other patch this weekend: I think we have enough
information to be able to continue searches without having to have
the shell crash for us.

pws

diff --git a/Src/hist.c b/Src/hist.c
index 82d03a840..7e6394406 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -1643,12 +1643,17 @@ hend(Eprog prog)
 void
 ihwbegin(int offset)
 {
+    int pos = hptr - chline + offset;
     if (stophist == 2 || (histactive & HA_INWORD) ||
 	(inbufflags & (INP_ALIAS|INP_HIST)) == INP_ALIAS)
 	return;
     if (chwordpos%2)
 	chwordpos--;	/* make sure we're on a word start, not end */
-    chwords[chwordpos++] = hptr - chline + offset;
+    DPUTS1(pos < 0, "History word position < 0 in %s",
+	   dupstrpfx(chline, hptr-chline));
+    if (pos < 0)
+	pos = 0;
+    chwords[chwordpos++] = pos;
 }
 
 /* Abort current history word, not needed */

Re: Bug: Searching through sufficiently large $historywords causes seg fault

[Bart Schaefer]

On Fri, May 12, 2023 at 2:28 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> Here's the only obvious case I can see in normal history management
> where you might get a miscalculation

On Wed, May 3, 2023 at 8:35 AM Sebastian Gniazdowski
<sgniazdowski@gmail.com> wrote:
>
> (gdb) p (int)(e-hstr)
> $65 = -31903

Is it not possible that what we actually have here is a huge overflow
in (e) such that (e-hstr) is a very large unsigned?