free() error on simple input scripts

free() error on simple input scripts

[Dennis Felsing]

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

Hello,

Simply running zsh (from git) on each of the two attached files causes a
free() error for me:

*** Error in `/usr/local/bin/zsh': free(): invalid next size (fast): 0x00000000009708c0 ***

This has been found fuzzing using AFL: http://lcamtuf.coredump.cx/afl/

Dennis

gdb output:

Program received signal SIGABRT, Aborted.
0x00007ffff6eeb5e7 in raise () from /lib64/libc.so.6
(gdb) bt
#0  0x00007ffff6eeb5e7 in raise () from /lib64/libc.so.6
#1  0x00007ffff6eec9c8 in abort () from /lib64/libc.so.6
#2  0x00007ffff6f2a0d4 in __libc_message () from /lib64/libc.so.6
#3  0x00007ffff6f2f9fe in malloc_printerr () from /lib64/libc.so.6
#4  0x00007ffff6f30716 in _int_free () from /lib64/libc.so.6
#5  0x00000000006fccfd in unmeta (file_name=0x7fffffffc1d0 "/media/intel/vtune_amplifier_xe_2011/bin64/d\203")
    at utils.c:4238
#6  0x0000000000473137 in iscom (s=0x7fffffffc1d0 "/media/intel/vtune_amplifier_xe_2011/bin64/d\203")
    at exec.c:824
#7  hashcmd (arg0=0x7ffff7ff3560 "d\203", pp=0x969168, pp@entry=0x969160) at exec.c:878
#8  0x0000000000489575 in execcmd (state=0x7fffffffd820, input=0, output=0, how=<optimized out>, last1=2)
    at exec.c:2886
#9  0x000000000049059e in execpline2 (state=0x7fffffffd820, pcode=16729, pcode@entry=131, how=18, input=0, 
    output=0, last1=5798544, last1@entry=0) at exec.c:1698
#10 0x0000000000491294 in execpline (state=state@entry=0x7fffffffd820, slcode=<optimized out>, 
    how=how@entry=18, last1=<optimized out>) at exec.c:1485
#11 0x00000000004952ab in execlist (state=state@entry=0x7fffffffd820, dont_change_job=dont_change_job@entry=0, 
    exiting=exiting@entry=0) at exec.c:1268
#12 0x0000000000495ce7 in execode (p=0x7ffff7ff33f0, dont_change_job=0, exiting=0, context=0x72a141 "toplevel")
    at exec.c:1074
#13 0x0000000000515861 in loop (toplevel=1, justonce=0) at init.c:185
#14 zsh_main (argc=<optimized out>, argv=<optimized out>) at init.c:1649
#15 0x00007ffff6ed7dc5 in __libc_start_main () from /lib64/libc.so.6
#16 0x0000000000413829 in _start ()

[-- Attachment #2: id:000000,sig:06,src:000000,op:havoc,rep:16 --]
[-- Type: application/octet-stream, Size: 26 bytes --]

[-- Attachment #3: id:000001,sig:06,src:000002,op:havoc,rep:8 --]
[-- Type: application/octet-stream, Size: 7 bytes --]

Re: free() error on simple input scripts

[Bart Schaefer]

On Dec 6,  5:27am, Dennis Felsing wrote:
}
} Simply running zsh (from git) on each of the two attached files causes a
} free() error for me:

These are both unicode files, at least one in 16-bit with a byte-order
prefix, and are therefore not valid input to the shell.

If you're in a situation where you're being caused to feed the shell
unknown or invalid input, you're already way worse off than can be
helped by avoiding a bad free() ...

However, it appears that both unmeta() and unmetafy() have trouble with
this input, e.g., unmeta() sees a META byte immediately before the end
of string NUL and therefore runs off the end at the second *t++ in this
loop:

    for (t = file_name, p = fn; *t; p++)
	if ((*p = *t++) == Meta)
	    *p = *t++ ^ 32;

This ought to get caught well before we reach this part of the function,
but I'm not sure what the correct reaction is.  Anyway, the failure of
unmeta[fy] cascades into errors in metafy() later.

Maybe this?  Though how we ended up with a bad metafied string in the
first place might also be worth investigating.

diff --git a/Src/utils.c b/Src/utils.c
index 9268147..5c90638 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -4164,7 +4164,7 @@ unmetafy(char *s, int *len)
 
     for (p = s; *p && *p != Meta; p++);
     for (t = p; (*t = *p++);)
-	if (*t++ == Meta)
+	if (*t++ == Meta && *p)
 	    t[-1] = *p++ ^ 32;
     if (len)
 	*len = t - s;
@@ -4208,8 +4208,10 @@ unmeta(const char *file_name)
     
     meta = 0;
     for (t = file_name; *t; t++) {
-	if (*t == Meta)
-	    meta = 1;
+	if (*t == Meta) {
+	    meta = t[1];
+	    break;
+	}
     }
     if (!meta) {
 	/*

Re: free() error on simple input scripts

[Bart Schaefer]

On Dec 6,  3:07pm, Bart Schaefer wrote:
}
} Though how we ended up with a bad metafied string in the
} first place might also be worth investigating.

I can reproduce the bad free with

    source =(<<<$'d\\\0')

There seems to be some problem with lexing the sequence backslash-nul,
which results in the bad meta-pair.  I haven't been able to isolate.

Re: free() error on simple input scripts

[Jun T.]

After the commit
48cd1b6c3b51a7bc6d45d4aeb7ff8c55d97a6f2a
    33894: boundary conditions in unmeta(), unmetafy()

test D07multibyte fails on my Mac as follows:

./D07multibyte.ztst: starting.
Testing multibyte with locale en_US.UTF-8
Test ./D07multibyte.ztst failed: bad status 134, expected 0 from:
  mkdir 梶浦由記 'Пётр Ильич Чайковский'
  (cd 梶浦由記; print ${${(%):-%~}:t})
  (cd 'Пётр Ильич Чайковский'; print ${${(%):-%~}:t})
Error output:
zsh(74285,0x7fff72bb3310) malloc: *** error for object 0x7f8162504998: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug
Was testing: Metafied characters in prompt expansion
./D07multibyte.ztst: test failed.

It seems the problem is in the following hunk:

> @@ -4208,8 +4208,10 @@ unmeta(const char *file_name)
> 
>     meta = 0;
>     for (t = file_name; *t; t++) {
> -	if (*t == Meta)
> -	    meta = 1;
> +	if (*t == Meta) {
> +	    meta = t[1];
> +	    break;
> +	}
>     }

Breaking out of the for-loop leaves the pointer t at the first Meta
in file_name, and the value of newsz becomes too small.

How about the following?

diff --git a/Src/utils.c b/Src/utils.c
index 5c90638..ab3d3da 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -4208,10 +4208,8 @@ unmeta(const char *file_name)
     
     meta = 0;
     for (t = file_name; *t; t++) {
-	if (*t == Meta) {
-	    meta = t[1];
-	    break;
-	}
+	if (*t == Meta)
+	    meta = 1;
     }
     if (!meta) {
 	/*
@@ -4250,7 +4248,7 @@ unmeta(const char *file_name)
     }
 
     for (t = file_name, p = fn; *t; p++)
-	if ((*p = *t++) == Meta)
+	if ((*p = *t++) == Meta && *t)
 	    *p = *t++ ^ 32;
     *p = '\0';
     return fn;

Re: free() error on simple input scripts

[Bart Schaefer]

On Dec 8,  9:51pm, Jun T. wrote:
}
} Breaking out of the for-loop leaves the pointer t at the first Meta
} in file_name, and the value of newsz becomes too small.
} 
} How about the following?

Yes, that would be better.  I didn't notice that t was being used to
compute a length.

Re: free() error on simple input scripts

[Jun T.]

2014/12/07 15:36, Bart Schaefer <schaefer@brasslantern.com> wrote:

>    source =(<<<$'d\\\0')

The input backslash-null is metafiled by shingetline() to
backslash-meta-space (since 0 xor 32 = 32 = ' '), but it seems
the lexer does not treat the meta after backslash specially and
interprets the space as a word separator; this results in a
word ending with meta.

What is the "correct" behavior for the input backslash-null?
The following may be a possibility but I'm not sure.
(The ifdef DEBUG part is copied from line 1059 in the same file)


diff --git a/Src/lex.c b/Src/lex.c
index 1a854f5..b2a0544 100644
--- a/Src/lex.c
+++ b/Src/lex.c
@@ -1326,8 +1326,20 @@ gettokstr(int c, int sub)
 		c = hgetc();
 		if (!lexstop)
 		    continue;
-	    } else
+	    } else {
 		add(Bnull);
+		if (c == STOUC(Meta)) {
+		    c = hgetc();
+#ifdef DEBUG
+		    if (lexstop) {
+			fputs("BUG: input terminated by Meta\n", stderr);
+			fflush(stderr);
+			goto brk;
+		    }
+#endif
+		    add(Meta);
+		}
+	    }
 	    if (lexstop)
 		goto brk;
 	    break;

Re: free() error on simple input scripts

[Peter Stephenson]

On Wed, 10 Dec 2014 00:45:42 +0900
Jun T. <takimoto-j@kba.biglobe.ne.jp> wrote:
> 2014/12/07 15:36, Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> >    source =(<<<$'d\\\0')
> 
> The input backslash-null is metafiled by shingetline() to
> backslash-meta-space (since 0 xor 32 = 32 = ' '), but it seems
> the lexer does not treat the meta after backslash specially and
> interprets the space as a word separator; this results in a
> word ending with meta.
> 
> What is the "correct" behavior for the input backslash-null?
> The following may be a possibility but I'm not sure.
> (The ifdef DEBUG part is copied from line 1059 in the same file)

I *think* your patch is correct.

Certainly, it is valid to get metafied input at the lexer.  See for
example the end of zleread() which calls zlegetline() which calls
zlelineasstring (typically --- unless we already have a metafied line)
which metafies the line that's about to be returned.

I'm a bit surprised the lexer doesn't have more cases like this, or a
more general way of dealing with metafied characters, but handling a
backslash the way you suggest looks perfectly OK.  However, wait and see
what Bart says, too.

pws

Re: free() error on simple input scripts

[Bart Schaefer]

On Dec 10, 12:45am, Jun T. wrote:
}
} What is the "correct" behavior for the input backslash-null?

Meta should always be Meta, even after a backslash; the whole point
of the Meta byte is that it introduces a two-character pair that
represents a single character, and the backslash should precede
whatever the represented character is, resulting in a 3-byte token
when backslash means "quote the next character" (which I think it
always does mean during gettokstr).

} The following may be a possibility but I'm not sure.
} (The ifdef DEBUG part is copied from line 1059 in the same file)

I agree with Peter that this patch looks correct.