.zsh_history bugreport

.zsh_history bugreport

[Stepan Koltsov]

[-- Attachment #1: Type: text/plain, Size: 422 bytes --]

Hi, all.

I don't know why, I got wrong .zsh_history file on my computer,
after that shell couldn't start, saying "fatal error: out of memory".

AFAIU, error is in hist.c:readhistline() -- it uses wrong algorithm.
Unfortunately, I have no time to fix that, and I hope, U'll do :-)

Bug was tested on zsh 4.1.0-dev-2 and 3.1.9-dev-6.

U can find my bad .zsh_history file in attachment.

-- 
mailto:yozh@mx1.ru
ICQ:26521795

[-- Attachment #2: .zsh_history.bad --]
[-- Type: application/octet-stream, Size: 24900 bytes --]

Re: .zsh_history bugreport

[Bart Schaefer]

This was fixed by the patch in users/4269 and should be working in the
zsh-4.0.4 release from earlier today.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: .zsh_history bugreport

[Stepan Koltsov]

On Fri, Oct 26, 2001 at 02:54:10PM +0000, Bart Schaefer wrote:
> This was fixed by the patch in users/4269 and should be working in the
> zsh-4.0.4 release from earlier today.

What was fixed? With same .zsh_history zsh get exits with same error.
And function readhistline() was not changed...

-- 
mailto:yozh@mx1.ru
ICQ:26521795

... Они жили долго и счастливо и надоели друг другу в один день...

Re: .zsh_history bugreport

[Wayne Davison]

On Fri, 26 Oct 2001, Bart Schaefer wrote:
> This was fixed by the patch in users/4269 and should be working in the
> zsh-4.0.4 release from earlier today.

Hmm.  The start >= l (ell) check can only error-out if the null byte
is at the start of the line (since once a section gets measured, it
can't ever get any shorter).

I think the real fix is to check if the string is too short when we
didn't find a newline at the end (i.e. the buffer should be maxed out
when fgets really didn't read a newline).  What's currently happening
is that we're doubling the readline buffer every time we get a short
read, and we may have only really added a few bytes to the buffer.

I think this is the right fix:

Index: Src/hist.c
--- Src/hist.c	2001/10/15 18:42:52	1.35
+++ Src/hist.c	2001/10/26 18:09:54
@@ -1772,11 +1772,10 @@
     if (fgets(buf + start, *bufsiz - start, in)) {
 	int l = strlen(buf);
 
-	if (start >= l)
-	    return -1;
-
 	if (l) {
 	    if (buf[l - 1] != '\n' && !feof(in)) {
+		if (l < *bufsiz - 1)
+		    return -1;
 		*bufp = zrealloc(buf, 2 * (*bufsiz));
 		*bufsiz = 2 * (*bufsiz);
 		return readhistline(l, bufp, bufsiz, in);

..wayne..

Re: .zsh_history bugreport

[Bart Schaefer]

On Oct 26, 11:26am, Wayne Davison wrote:
> Subject: Re: .zsh_history bugreport
> On Fri, 26 Oct 2001, Bart Schaefer wrote:
> > This was fixed by the patch in users/4269 and should be working in the
> > zsh-4.0.4 release from earlier today.
> 
> Hmm.  The start >= l (ell) check can only error-out if the null byte
> is at the start of the line (since once a section gets measured, it
> can't ever get any shorter).

No, that's not true.  Look at the recursive call.  The value passed in
for start is the value of the previous l.  So if fgets() returns nonzero
but buf didn't get any longer, start == l and (start >= l) is true and so
we return -1.  The greater-than check is just in there for completeness;
only the equivalence should happen in practice.

It doesn't matter where the null byte is.

Re: .zsh_history bugreport

[Wayne Davison]

On Fri, 26 Oct 2001, Bart Schaefer wrote:
> So if fgets() returns nonzero but buf didn't get any longer, start ==
> l and (start >= l) is true and so we return -1.

The problem occurs when the buf DOES get longer, even just 1 character
longer.  In this case, the '\0' byte was not at the start of the line,
so we double the readline buffer, and recurse (even though we're nowhere
near the limit of our readline buffer).  As long as the '\0' byte is not
immidiately followed by a newline byte, we continue to read.

My original patch was wrong in that it did not handle the case where
the '\0' was at the very start of the first read, so I've fixed that.

I also noticed that the existing code had a problem in the rare case
where we read a partial string (w/o a newline) because of an EOF: the
code would eliminate the last character of the string, even though it
was not a newline.

Finally, I optimized the logic a tiny bit, and since "l" (ell) looks so
much like a "1" (one), I changed that variable name to be "len".  Here's
my new patch.

..wayne..

---8<------8<------8<------8<---cut here--->8------>8------>8------>8---
Index: Src/hist.c
--- Src/hist.c	2001/10/15 18:42:52	1.35
+++ Src/hist.c	2001/10/26 23:27:28
@@ -1766,31 +1766,34 @@
 
 static int histfile_linect;
 
-static int readhistline(int start, char **bufp, int *bufsiz, FILE *in)
+static int
+readhistline(int start, char **bufp, int *bufsiz, FILE *in)
 {
     char *buf = *bufp;
     if (fgets(buf + start, *bufsiz - start, in)) {
-	int l = strlen(buf);
-
-	if (start >= l)
+	int len = start + strlen(buf + start);
+	if (len == start)
 	    return -1;
-
-	if (l) {
-	    if (buf[l - 1] != '\n' && !feof(in)) {
+	if (buf[len - 1] != '\n') {
+	    if (!feof(in)) {
+		if (len < (*bufsiz) - 1)
+		    return -1;
 		*bufp = zrealloc(buf, 2 * (*bufsiz));
 		*bufsiz = 2 * (*bufsiz);
-		return readhistline(l, bufp, bufsiz, in);
+		return readhistline(len, bufp, bufsiz, in);
 	    }
-	    buf[l - 1] = '\0';
-	    if (l > 1 && buf[l - 2] == '\\') {
-		buf[--l - 1] = '\n';
+	}
+	else {
+	    buf[len - 1] = '\0';
+	    if (len > 1 && buf[len - 2] == '\\') {
+		buf[--len - 1] = '\n';
 		if (!feof(in))
-		    return readhistline(l, bufp, bufsiz, in);
+		    return readhistline(len, bufp, bufsiz, in);
 	    }
 	}
-	return l;
-    } else
-	return 0;
+	return len;
+    }
+    return 0;
 }
 
 /**/
---8<------8<------8<------8<---cut here--->8------>8------>8------>8---

Re: .zsh_history bugreport

[Bart Schaefer]

On Oct 26,  4:29pm, Wayne Davison wrote:
} Subject: Re: .zsh_history bugreport
}
} On Fri, 26 Oct 2001, Bart Schaefer wrote:
} > So if fgets() returns nonzero but buf didn't get any longer, start ==
} > l and (start >= l) is true and so we return -1.
} 
} The problem occurs when the buf DOES get longer, even just 1 character
} longer.  In this case, the '\0' byte was not at the start of the line,

OK, I concur (though I'm surprised that every one of the assorted binary
files I tried feeding to my version had at least one "\n\0" in it).

16184 probably should go onto the patches branch, too.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: .zsh_history bugreport

[Wayne Davison]

On Tue, 30 Oct 2001, Bart Schaefer wrote:
> 16184 probably should go onto the patches branch, too.

Cool.  I just put it there.

One other potential change:  Should readhistline() return "start"
instead of "0" when fgets() fails?  Having fgets() fail with a non-0
start value doesn't look like a very common scenario (because of the
feof() calls), but if we've got some characters in the string, I figure
we might as well return them.  Do you agree?

Index: Src/hist.c
--- Src/hist.c	2001/10/26 23:47:10	1.36
+++ Src/hist.c	2001/10/30 18:19:46
@@ -1793,7 +1793,7 @@
 	}
 	return len;
     }
-    return 0;
+    return start;
 }
 
 /**/

..wayne..

Re: .zsh_history bugreport

[Bart Schaefer]

On Oct 30, 10:25am, Wayne Davison wrote:
}
} One other potential change: Should readhistline() return "start"
} instead of "0" when fgets() fails? [...] if we've got some characters
} in the string, I figure we might as well return them. Do you agree?

I considered that when first rewriting readhistline(), but ...

I think it's more likely that if the history file is corrupted, then we
should treat the line as entirely unusable, rather than possibly push
garbage into the history list.  It's not overtly harmful, of course,
since history entries don't get executed directly, but once there's junk
in the history it may be confusing for many users to figure out how to
get it *out* of the history again (particularly without losing what comes
before and after it).

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net