infinite recursion when handling the "out of memory" state

infinite recursion when handling the "out of memory" state

[Kamil Dudka]

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

If zsh is compiled with multibyte support, handling of the "out of memory" 
state does not work well in certain cases -- instead of printing the error 
message and exiting, zsh ends up in an infinite recursion and crashes due to 
stack overflow.

The memory allocation functions in mem.c use zerr() to print the fatal error 
messages.  However, zerr() calls zwarning() and transitively mb_niceformat(), 
which allocates heap memory (and may call zerr() on failure).

I see three options how to prevent the stack overflow in such cases:

1. avoid using zerr() to print "out of memory" error messages

2. implement zerr() such that it does not trigger heap memory allocation
(I am afraid this is not really possible)

3. introduce a flag that would prevent zerr() from recurring into itself

I have attached a reproducer script.  It works reliably if the zsh executable 
is linked statically.  Otherwise it may happen that the dynamic linker is more 
hungry on memory than zsh itself, which would hide the bug.

Originally reported at: https://bugzilla.redhat.com/1300958

Kamil

[-- Attachment #2: oom.zsh --]
[-- Type: application/x-shellscript, Size: 699 bytes --]

Re: infinite recursion when handling the "out of memory" state

[Peter Stephenson]

On Mon, 25 Jan 2016 16:02:50 +0100
Kamil Dudka <kdudka@redhat.com> wrote:
> If zsh is compiled with multibyte support, handling of the "out of memory" 
> state does not work well in certain cases -- instead of printing the error 
> message and exiting, zsh ends up in an infinite recursion and crashes due to 
> stack overflow.
> 
> The memory allocation functions in mem.c use zerr() to print the fatal error 
> messages.  However, zerr() calls zwarning() and transitively mb_niceformat(), 
> which allocates heap memory (and may call zerr() on failure).
> 
> 3. introduce a flag that would prevent zerr() from recurring into itself

I think this is easy --- we already do this except that the flag is set
after the warning is printed rather than before.  The zwarning()
function that is called in the middle isn't directly sensitive to the
flag, so moving the code up should have the desired effect and is the
sane thing to do with error messages anyway.

diff --git a/Src/utils.c b/Src/utils.c
index fd0bab3..17ebfeb 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -169,12 +169,12 @@ VA_DCL
 	    errflag |= ERRFLAG_ERROR;
 	return;
     }
+    errflag |= ERRFLAG_ERROR;
 
     VA_START(ap, fmt);
     VA_GET_ARG(ap, fmt, const char *);
     zwarning(NULL, fmt, ap);
     va_end(ap);
-    errflag |= ERRFLAG_ERROR;
 }
 
 /**/
@@ -188,13 +188,13 @@ VA_DCL
 
     if (errflag || noerrs)
 	return;
+    errflag |= ERRFLAG_ERROR;
 
     VA_START(ap, fmt);
     VA_GET_ARG(ap, cmd, const char *);
     VA_GET_ARG(ap, fmt, const char *);
     zwarning(cmd, fmt, ap);
     va_end(ap);
-    errflag |= ERRFLAG_ERROR;
 }
 
 /**/

Re: infinite recursion when handling the "out of memory" state

[Kamil Dudka]

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

On Monday 25 January 2016 15:31:25 Peter Stephenson wrote:
> On Mon, 25 Jan 2016 16:02:50 +0100
> 
> Kamil Dudka <kdudka@redhat.com> wrote:
> > If zsh is compiled with multibyte support, handling of the "out of memory"
> > state does not work well in certain cases -- instead of printing the error
> > message and exiting, zsh ends up in an infinite recursion and crashes due
> > to stack overflow.
> > 
> > The memory allocation functions in mem.c use zerr() to print the fatal
> > error messages.  However, zerr() calls zwarning() and transitively
> > mb_niceformat(), which allocates heap memory (and may call zerr() on
> > failure).
> > 
> > 3. introduce a flag that would prevent zerr() from recurring into itself
> 
> I think this is easy --- we already do this except that the flag is set
> after the warning is printed rather than before.  The zwarning()
> function that is called in the middle isn't directly sensitive to the
> flag, so moving the code up should have the desired effect and is the
> sane thing to do with error messages anyway.

Thanks!  Your patch indeed prevented the infinite recursion from occurring.
I am attaching one more OOM fix to make zerr() work in case argzero is NULL,
like in the following backtrace:

#0  0x000000000041ebe2 in untokenize (s=0x0) at exec.c:1857
No locals.
#1  0x000000000049971e in mb_niceformat (s=0x0, stream=0x880580 <_IO_2_1_stderr_>, outstrp=0x0, flags=0) at utils.c:4994
        l = 0
        newl = 5598153
        umlen = 0
        outalloc = 0
        outleft = 0
        eol = 0
        c = 22 L'\026'
        ums = 0x0
        ptr = 0x16 <error: Cannot access memory at address 0x16>
        fmt = 0x170f12001a131100 <error: Cannot access memory at address 0x170f12001a131100>
        outstr = 0x0
        outptr = 0x0
        mbs = {__count = 2132542208, __value = {__wch = 16778261, __wchb = "\025\004\000\001"}}
#2  0x000000000048fc15 in zwarning (cmd=0x0, fmt=0x5f6228 "fatal error: out of memory", ap=0x7fff1b83b5f8) at utils.c:150
No locals.
#3  0x000000000048fd26 in zerr (fmt=0x5f6228 "fatal error: out of memory") at utils.c:176
        ap = {{gp_offset = 8, fp_offset = 48, overflow_arg_area = 0x7fff1b83b6d0, reg_save_area = 0x7fff1b83b610}}
#4  0x00000000004559af in zshcalloc (size=64) at mem.c:975
        ptr = 0x0
#5  0x00000000004415d7 in zsh_main (argc=3, argv=0x7fff1b83b958) at init.c:1651
        t = 0x7fff1b83b970
        runscript = 0x0
        cmd = 0x7fff1b83b978 ",\301\203\033\377\177"
        t0 = 162
#6  0x0000000000400bbe in main (argc=3, argv=0x7fff1b83b958) at ./main.c:93
No locals.

[-- Attachment #2: 0001-prevent-zerr-from-crashing-when-printing-out-of-memo.patch --]
[-- Type: text/x-patch, Size: 1269 bytes --]

>From 8636c26cfb8d2b4b61b7bbd802042910b707ebc2 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Mon, 25 Jan 2016 17:53:41 +0100
Subject: [PATCH] prevent zerr() from crashing when printing "out of memory"

... in case argzero is NULL
---
 Src/utils.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Src/utils.c b/Src/utils.c
index 17ebfeb..053731c 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -133,9 +133,11 @@ zwarning(const char *cmd, const char *fmt, va_list ap)
     if (isatty(2))
 	zleentry(ZLE_CMD_TRASH);
 
+    char *prefix = scriptname ? scriptname : (argzero ? argzero : "");
+
     if (cmd) {
 	if (unset(SHINSTDIN) || locallevel) {
-	    nicezputs(scriptname ? scriptname : argzero, stderr);
+	    nicezputs(prefix, stderr);
 	    fputc((unsigned char)':', stderr);
 	}
 	nicezputs(cmd, stderr);
@@ -147,8 +149,7 @@ zwarning(const char *cmd, const char *fmt, va_list ap)
 	 * program/script is running.  It's also set in shell functions,
 	 * so test locallevel, too.
 	 */
-	nicezputs((isset(SHINSTDIN) && !locallevel) ? "zsh" :
-		  scriptname ? scriptname : argzero, stderr);
+	nicezputs((isset(SHINSTDIN) && !locallevel) ? "zsh" : prefix, stderr);
 	fputc((unsigned char)':', stderr);
     }
 
-- 
2.5.0