Re: [BUG] malloc inside signal handler

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

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);