[BUG] malloc inside signal handler

[BUG] malloc inside signal handler

[zsugabubus]

Hi everybody,

It seems that setiparam("LINES"/"COLUMNS") inside adjustwinsize() wants
allocate some memory for environment variables.

    #0  0x00007ffff7c5ad22 raise (libc.so.6 + 0x3cd22)
    #1  0x00007ffff7c44862 abort (libc.so.6 + 0x26862)
    #2  0x00007ffff7ca4b08 __malloc_assert (libc.so.6 + 0x86b08)
    #3  0x00007ffff7ca6bf3 sysmalloc (libc.so.6 + 0x88bf3)
    #4  0x00007ffff7ca840e _int_malloc (libc.so.6 + 0x8a40e)
    #5  0x00007ffff7ca9397 malloc (libc.so.6 + 0x8b397)
    #6  0x00005555555ba7fb zalloc (zsh + 0x667fb)
    #7  0x00005555555c29a5 n/a (zsh + 0x6e9a5)
    #8  0x00005555555c4735 addenv (zsh + 0x70735)
    #9  0x00005555555c86cc export_param (zsh + 0x746cc)
    #10 0x00005555555c8b10 n/a (zsh + 0x74b10)
    #11 0x00005555555cb912 setnumvalue (zsh + 0x77912)
    #12 0x00005555555cc59d assignnparam (zsh + 0x7859d)
    #13 0x00005555555f20ba adjustwinsize (zsh + 0x9e0ba)
    #14 0x00005555555e138b zhandler (zsh + 0x8d38b)
    #15 0x00007ffff7c5ada0 __restore_rt (libc.so.6 + 0x3cda0)
    #16 0x00007ffff7ca83ab _int_malloc (libc.so.6 + 0x8a3ab)
    #17 0x00007ffff7ca9397 malloc (libc.so.6 + 0x8b397)
    #18 0x00007ffff7c92564 _IO_file_doallocate (libc.so.6 + 0x74564)
    #19 0x00007ffff7ca0db0 _IO_doallocbuf (libc.so.6 + 0x82db0)
    #20 0x00007ffff7c9fcbc _IO_file_underflow@@GLIBC_2.2.5 (libc.so.6 + 0x81cbc)
    #21 0x00007ffff7ca0e66 _IO_default_uflow (libc.so.6 + 0x82e66)
    #22 0x00005555555a8463 shingetline (zsh + 0x54463)
    #23 0x00005555555a8a29 n/a (zsh + 0x54a29)
    #24 0x00005555555b2666 n/a (zsh + 0x5e666)
    #25 0x00005555555d2f17 parse_event (zsh + 0x7ef17)
    #26 0x00005555555a3f3f loop (zsh + 0x4ff3f)
    #27 0x00005555555a7d36 zsh_main (zsh + 0x53d36)
    #28 0x00007ffff7c45b25 __libc_start_main (libc.so.6 + 0x27b25)
    #29 0x000055555556b0ae _start (zsh + 0x170ae)

$ zsh --version
zsh 5.8 (x86_64-pc-linux-gnu)

Thanks,
zsugabubus

Re: [BUG] malloc inside signal handler

[Bart Schaefer]

On Sat, Jul 24, 2021 at 3:52 PM zsugabubus
<zsugabubus@national.shitposting.agency> wrote:
>
> It seems that setiparam("LINES"/"COLUMNS") inside adjustwinsize() wants
> allocate some memory for environment variables.

This statement is true, but has been true for decades; we go to some
lengths to make sure that signal handlers are not invoked during other
operations that perform malloc, so that there is no re-entry issue if
the signal handler needs to perform allocation.  It's not possible to
run shell-code-based signal handlers without any means of dynamic
memory management.

Under what circumstances are you generating this error?

Re: [BUG] malloc inside signal handler

[zsugabubus]

On Fri, Jul 30, 2021 at 06:33:08PM -0700, Bart Schaefer wrote:
> Under what circumstances are you generating this error?

I had the crash with the following command:

    zsh -c ". hashes.zsh; print ~dir"

that was being executed inside $TERMINAL -e so that explains SIGWINCH.

(hashes.zsh contains a few hash -d lines.)

Unfortunately, I could not find any way to reproduce this again, even
though I call it a few hundred times a day.

However, looking at the stack trace it seems straightforward (for me)
what's going on:

shingetline() calls winch_unblock() and dont_queue_signals() so fgetc()
can be interrupted at any time.

$ gdb zsh -ex 'b input.c:153' -ex 'r' -ex 'b adjustwinsize' -ex 'signal SIGWINCH' -ex 'bt'

Just to make sure, it confirms that SIGWINCH can be handled around
fgetc(), maybe during a memory allocation inside it so if
adjustwinsize() also uses malloc() that can be fatal.

-- 
zsugabubus

Re: [BUG] malloc inside signal handler

[Bart Schaefer]

I just found this in my spam folder.

On Sat, Jul 31, 2021 at 1:25 PM zsugabubus
<zsugabubus@national.shitposting.agency> wrote:
>
> shingetline() calls winch_unblock() and dont_queue_signals() so fgetc()
> can be interrupted at any time.

Hm.  I'm not sure what we can do about this, except to stop using
stdio, because we have to be able to react to window size changes
while blocked waiting for input (that would be the most common time
for a window to be resized).  Maybe there's something sneaky we can do
with read_poll() so that we don't call fgetc() until it's guaranteed
to return something, and then we could hold signals again around the
actual fgetc() call.

Or maybe shingetline() doesn't actually need to call winch_unblock()
when loop(0,...) is called and isatty(bshin) is false ... which means
passing the value of toplevel down from loop().  But that probably
just fixes adjustwinsize() and leaves other signal handlers
unprotected.

Re: [BUG] malloc inside signal handler

[Peter Stephenson]

> On 17 August 2021 at 05:20 Bart Schaefer <schaefer@brasslantern.com> wrote:
> Or maybe shingetline() doesn't actually need to call winch_unblock()
> when loop(0,...) is called and isatty(bshin) is false ... which means
> passing the value of toplevel down from loop().  But that probably
> just fixes adjustwinsize() and leaves other signal handlers
> unprotected.

Might be worth doing anyway, since adjustwinsize() is the one that's doing
the big allocation within the signal code.  I haven't checked, but I
doubt there's anything like that in the others.

Avoiding stdio for input of this sort doesn't actually look infeasible,
either --- mentions of bshin (buffered shell input from files) are
restricted to only a couple of files.  But it's kind of annoying as this
is exactly the job stdio is designed for.

pws

Re: [BUG] malloc inside signal handler

[Peter Stephenson]

On Tue, 2021-08-17 at 09:21 +0100, Peter Stephenson wrote:
> Avoiding stdio for input of this sort doesn't actually look infeasible,
> either --- mentions of bshin (buffered shell input from files) are
> restricted to only a couple of files.  But it's kind of annoying as this
> is exactly the job stdio is designed for.

As I noted, this isn't that difficult, and this should remove the
problem --- all the memory management is now in places where we would
expect it.

I don't know what the general feeling is about this, but as a piece of
long-term robustness and with no immediately upcoming release I'd
incline to getting this or something like it in for trying out.

It could usefully do with a scan for leaks, which is the most likely
thing I can see going wrong.  Obviously I've checked myself, but
potentially there are possible leaks of file descriptors, stack entries,
and allocated buffers, so it's not completely trivial.

In case it's not clear: the allocation of the buffer is not tied to the
file descriptor in use.  The only time we need to allocate a new buffer
other than at initialisation is when we're temporarily saving the
existing input while we source a different file.  Otherwise, we simply
need to reset the buffer pointers.

pws

diff --git a/Src/init.c b/Src/init.c
index 3d6c94d04..878a53a37 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -1229,7 +1229,7 @@ setupshin(char *runscript)
     /*
      * Finish setting up SHIN and its relatives.
      */
-    bshin = SHIN ? fdopen(SHIN, "r") : stdin;
+    shinbufalloc();
     if (isset(SHINSTDIN) && !SHIN && unset(INTERACTIVE)) {
 #ifdef _IONBF
 	setvbuf(stdin, NULL, _IONBF, 0);
@@ -1384,9 +1384,9 @@ init_misc(char *cmd, char *zsh_name)
 	dosetopt(RESTRICTED, 1, 0, opts);
     if (cmd) {
 	if (SHIN >= 10)
-	    fclose(bshin);
+	    close(SHIN);
 	SHIN = movefd(open("/dev/null", O_RDONLY | O_NOCTTY));
-	bshin = fdopen(SHIN, "r");
+	shinbufreset();
 	execstring(cmd, 0, 1, "cmdarg");
 	stopmsg = 1;
 	zexit((exit_pending || shell_exiting) ? exit_val : lastval, ZEXIT_NORMAL);
@@ -1409,7 +1409,6 @@ source(char *s)
     int tempfd = -1, fd, cj;
     zlong oldlineno;
     int oldshst, osubsh, oloops;
-    FILE *obshin;
     char *old_scriptname = scriptname, *us;
     char *old_scriptfilename = scriptfilename;
     unsigned char *ocs;
@@ -1426,7 +1425,6 @@ source(char *s)
 
     /* save the current shell state */
     fd        = SHIN;            /* store the shell input fd                  */
-    obshin    = bshin;          /* store file handle for buffered shell input */
     osubsh    = subsh;           /* store whether we are in a subshell        */
     cj        = thisjob;         /* store our current job number              */
     oldlineno = lineno;          /* store our current lineno                  */
@@ -1439,7 +1437,7 @@ source(char *s)
 
     if (!prog) {
 	SHIN = tempfd;
-	bshin = fdopen(SHIN, "r");
+	shinbufsave();
     }
     subsh  = 0;
     lineno = 1;
@@ -1507,10 +1505,10 @@ source(char *s)
     if (prog)
 	freeeprog(prog);
     else {
-	fclose(bshin);
+	close(SHIN);
 	fdtable[SHIN] = FDT_UNUSED;
 	SHIN = fd;		     /* the shell input fd                   */
-	bshin = obshin;		     /* file handle for buffered shell input */
+	shinbufrestore();
     }
     subsh = osubsh;                  /* whether we are in a subshell         */
     thisjob = cj;                    /* current job number                   */
diff --git a/Src/input.c b/Src/input.c
index f568cc135..3c6ad7002 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -80,11 +80,6 @@
 /**/
 int SHIN;
 
-/* buffered shell input for non-interactive shells */
-
-/**/
-FILE *bshin;
-
 /* != 0 means we are reading input from a string */
  
 /**/
@@ -129,7 +124,116 @@ static struct instacks *instack, *instacktop;
 
 static int instacksz = INSTACK_INITIAL;
 
-/* Read a line from bshin.  Convert tokens and   *
+/* Size of buffer for non-interactive command input */
+
+#define SHINBUFSIZE 8192
+
+/* Input buffer for non-interactive command input */
+static char *shinbuffer;
+
+/* Pointer into shinbuffer */
+static char *shinbufptr;
+
+/* End of contents read into shinbuffer */
+static char *shinbufendptr;
+
+/* Entry on SHIN buffer save stack */
+struct shinsaveentry {
+    /* Next entry on stack */
+    struct shinsaveentry *next;
+    /* Saved shinbuffer */
+    char *buffer;
+    /* Saved shinbufptr */
+    char *ptr;
+    /* Saved shinbufendptr */
+    char *endptr;
+};
+
+/* SHIN buffer save stack */
+struct shinsaveentry *shinsavestack;
+
+/* Reset the input buffer for SHIN, discarding any pending input */
+
+/**/
+void
+shinbufreset(void)
+{
+    shinbufendptr = shinbufptr = shinbuffer;
+}
+
+/* Allocate a new shinbuffer
+ *
+ * Only called at shell initialisation and when saving on the stack.
+ */
+
+/**/
+void
+shinbufalloc(void)
+{
+    shinbuffer = zalloc(SHINBUFSIZE);
+    shinbufreset();
+}
+
+/* Save entry on SHIN buffer save stack */
+
+/**/
+void
+shinbufsave(void)
+{
+    struct shinsaveentry *entry =
+	(struct shinsaveentry *)zalloc(sizeof(struct shinsaveentry));
+
+    entry->next = shinsavestack;
+    entry->buffer = shinbuffer;
+    entry->ptr = shinbufptr;
+    entry->endptr = shinbufendptr;
+
+    shinsavestack = entry;
+
+    shinbufalloc();
+}
+
+/* Restore entry from SHIN buffer save stack */
+
+/**/
+void
+shinbufrestore(void)
+{
+    struct shinsaveentry *entry = shinsavestack;
+
+    zfree(shinbuffer, SHINBUFSIZE);
+
+    shinbuffer = entry->buffer;
+    shinbufptr = entry->ptr;
+    shinbufendptr = entry->endptr;
+
+    shinsavestack = entry->next;
+    zfree(entry, sizeof(struct shinsaveentry));
+}
+
+/* Get a character from SHIN, -1 if none available */
+
+/**/
+static int
+shingetchar(void)
+{
+    int nread;
+
+    if (shinbufptr < shinbufendptr)
+	return STOUC(*shinbufptr++);
+
+    shinbufreset();
+    do {
+	errno = 0;
+	nread = read(SHIN, shinbuffer, SHINBUFSIZE);
+    } while (nread < 0 && errno == EINTR);
+    if (nread <= 0)
+	return -1;
+    shinbufendptr = shinbuffer + nread;
+    return STOUC(*shinbufptr++);
+}
+
+/* Read a line from SHIN.  Convert tokens and   *
  * null characters to Meta c^32 character pairs. */
 
 /**/
@@ -147,11 +251,7 @@ shingetline(void)
     winch_unblock();
     dont_queue_signals();
     for (;;) {
-	/* Can't fgets() here because we need to accept '\0' bytes */
-	do {
-	    errno = 0;
-	    c = fgetc(bshin);
-	} while (c < 0 && errno == EINTR);
+	c = shingetchar();
 	if (c < 0 || c == '\n') {
 	    winch_block();
 	    restore_queue_signals(q);

Re: [BUG] malloc inside signal handler

[Peter Stephenson]

> On 18 August 2021 at 20:27 Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> 
> 
> On Tue, 2021-08-17 at 09:21 +0100, Peter Stephenson wrote:
> > Avoiding stdio for input of this sort doesn't actually look infeasible,
> > either --- mentions of bshin (buffered shell input from files) are
> > restricted to only a couple of files.  But it's kind of annoying as this
> > is exactly the job stdio is designed for.
> 
> As I noted, this isn't that difficult, and this should remove the
> problem --- all the memory management is now in places where we would
> expect it.
> 
> I don't know what the general feeling is about this, but as a piece of
> long-term robustness and with no immediately upcoming release I'd
> incline to getting this or something like it in for trying out.

I'm not hearing any objections to this.  I've checked it through again myself,
and can't see any problems or dubious areas, and I've been using it myself
for a week, so I'll check it in after waiting to see if there are any last
minute comments.

pws

Re: [BUG] malloc inside signal handler

[Bart Schaefer]

On Thu, Aug 26, 2021 at 8:46 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> I'm not hearing any objections to this.  I've checked it through again myself,
> and can't see any problems or dubious areas, and I've been using it myself
> for a week, so I'll check it in after waiting to see if there are any last
> minute comments.

It seems fine to me.