From 411481eb042c693ed8f4fd1e043731c677a204dd Mon Sep 17 00:00:00 2001
From: Martijn Dekker <martijn@inlv.org>
Date: Mon, 20 Jun 2022 14:46:22 +0100
Subject: [PATCH] Fix "/dev/null: cannot create [Bad file descriptor]" (re:
 feeb62d1)

Reproducer for a race condition:

  typeset -i i
  cat=${ command -pv cat; }
  for((i=0;i<20000;i++))
  do    printf '\r%d ' "$i"
        ksh |&
        cop=$!
        print -p $'print hello | '$cat$'\nprint '$exp
        read -t 1 -p
        read -t 1 -p
        redirect 5<&p 6>&p 5<&- 6>&-
        { sleep .4; kill $cop; } &
        spy=$!
        if    wait $cop 2>/dev/null # "/dev/null: cannot create"?
        then  kill $spy
        else  kill $spy; exit       # boom
        fi
        wait
  done

Result on my system. The time it takes to fail varies a lot:

  $ arch/*/bin/ksh foo
  717 foo[13]: /dev/null: cannot create [Bad file descriptor]
  $

Analysis:

errno may get a value during SIGCHLD processing. This may cause the
functions called by the signal interrupt to call sh_close() with a
negative file descriptor, giving errno the EBADF value as this loop
in sh_open() is interrupted:

ksh/src/cmd/ksh93/sh/io.c in 94fc983
851:	 while((fd = open(path, flags, mode)) < 0)
852:	 	if(errno!=EINTR || sh.trapnote)
853: 			return(-1);

Commit feeb62d1 caused sh_close() to set errno to EBADF if it gets
called with a negative file descriptor. In the cleanup of
coprocesses during the SIGCHLD interrupt handling, there are
several locations where sh_close() may be called with a negative
file descriptor, which now sets errno to EBADF. If SIGCHLD is
handled between open() being interrupted and the check for
errno!=EINTR, sh_open() will return -1 instead of retrying, causing
sh_redirect() to issue the "cannot create" error message.

SIGCHLD is issued whenever a process (such as the coprocess in the
reproducer) terminates. job_reap() is called (via job_waitsafe())
to handle this. That function does not always restore errno, which
allowed this race condition to happen.

src/cmd/ksh93/sh/jobs.c: job_reap():
- Always save/restore errno.

Resolves: https://github.com/ksh93/ksh/issues/483
---
 src/cmd/ksh93/sh/jobs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/cmd/ksh93/sh/jobs.c b/src/cmd/ksh93/sh/jobs.c
index 27abc493b..a8f0d03bf 100644
--- a/src/cmd/ksh93/sh/jobs.c
+++ b/src/cmd/ksh93/sh/jobs.c
@@ -291,7 +291,7 @@ int job_reap(register int sig)
 	struct process *px;
 	register int flags;
 	struct jobsave *jp;
-	int nochild=0, oerrno, wstat;
+	int nochild = 0, oerrno = errno, wstat;
 	Waitevent_f waitevent = sh.waitevent;
 	static int wcontinued = WCONTINUED;
 	int was_ttywait_on;
@@ -312,7 +312,6 @@ int job_reap(register int sig)
 	else
 		flags = WUNTRACED|wcontinued;
 	sh.waitevent = 0;
-	oerrno = errno;
 	was_ttywait_on = sh_isstate(SH_TTYWAIT); /* save tty wait state */
 	while(1)
 	{
@@ -478,7 +477,6 @@ int job_reap(register int sig)
 	}
 	if(errno==ECHILD)
 	{
-		errno = oerrno;
 #if SHOPT_BGX
 		job.numbjob = 0;
 #endif /* SHOPT_BGX */
@@ -494,6 +492,14 @@ int job_reap(register int sig)
 	}
 	if(sig)
 		signal(sig, job_waitsafe);
+	/*
+	 * Always restore errno, because this code is run during signal handling which may interrupt loops like:
+	 *	while((fd = open(path, flags, mode)) < 0)
+	 *		if(errno!=EINTR)
+	 *			<throw error>;
+	 * otherwise that may fail if SIGCHLD is handled between the open() call and the errno!=EINTR check.
+	 */
+	errno = oerrno;
 	return(nochild);
 }