From a72ba2cf596542ad7978f02395d4f5da3a7bba17 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Mon, 20 Jun 2022 03:16:57 +0100 Subject: [PATCH] Fix "/dev/null: cannot create [Bad file descriptor]" (re: feeb62d1) Intermittent regression test failure: test coprocess begins at 2022-02-08+23:41:52 /usr/local/src/ksh93/ksh/src/cmd/ksh93/tests/coprocess.sh[116]: /dev/null: cannot create [Bad file descriptor] coprocess.sh[118]: FAIL: /bin/cat coprocess hung after 'exec 5<&p 6>&p; exec 5<&- 6>&-' test coprocess failed at 2022-02-08+23:41:56 with exit code 1 [ 33 tests 1 error ] The coprocess didn't hang; the 2>/dev/null redirection in the regression test somehow failed. Running the regression test in a loop 10000 times is a fairly reliable way of reproducing the failure on my system. This has allowed me to find the commit that introduced it. It is: feeb62d1 How odd. That commit merely sets errno to EBADF if sh_close() is called with a file descriptor < 0. I don't understand how this causes a race condition. But calling abort() in the location of the feeb62d1 patch did allow me to trace all the sh_close() calls with a negative file descriptor. Those shouldn't be happening in any case. All those sh_close() calls were related to co-processes, which that regression test also tests. A co-process pipe that is not in use is set to -1 so we need to avoid calling sh_close() if that happened. This commit does that (as well as making some consistency tweaks) and it reliably makes the race condition go away on my system. The "why" of this remains a mystery as the only difference is that errno is not set to EBADF in these situations, and both sh_redirect() and sh_open() explicitly set errno to 0. Resolves: https://github.com/ksh93/ksh/issues/483 --- src/cmd/ksh93/sh/io.c | 4 ++-- src/cmd/ksh93/sh/jobs.c | 9 +++++---- src/cmd/ksh93/sh/path.c | 7 +++++-- src/cmd/ksh93/sh/subshell.c | 6 ++++-- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/cmd/ksh93/sh/io.c b/src/cmd/ksh93/sh/io.c index 5db81ba3e..fc88cb0bc 100644 --- a/src/cmd/ksh93/sh/io.c +++ b/src/cmd/ksh93/sh/io.c @@ -1045,9 +1045,9 @@ static Sfoff_t file_offset(int fn, char *fname) */ void sh_pclose(register int pv[]) { - if(pv[0]>=2) + if(pv[0] > -1) sh_close(pv[0]); - if(pv[1]>=2) + if(pv[1] > -1) sh_close(pv[1]); pv[0] = pv[1] = -1; } diff --git a/src/cmd/ksh93/sh/jobs.c b/src/cmd/ksh93/sh/jobs.c index 27abc493b..0d55fe9b4 100644 --- a/src/cmd/ksh93/sh/jobs.c +++ b/src/cmd/ksh93/sh/jobs.c @@ -396,10 +396,11 @@ int job_reap(register int sig) /* check for coprocess completion */ if(pid==sh.cpid) { - sh_close(sh.coutpipe); - sh_close(sh.cpipe[1]); - sh.cpipe[1] = -1; - sh.coutpipe = -1; + if(sh.coutpipe > -1) + sh_close(sh.coutpipe); + if(sh.cpipe[1] > -1) + sh_close(sh.cpipe[1]); + sh.coutpipe = sh.cpipe[1] = -1; } else if(sh.subshell) sh_subjobcheck(pid); diff --git a/src/cmd/ksh93/sh/path.c b/src/cmd/ksh93/sh/path.c index e3bda4046..363a1debf 100644 --- a/src/cmd/ksh93/sh/path.c +++ b/src/cmd/ksh93/sh/path.c @@ -1275,10 +1275,13 @@ static noreturn void exscript(register char *path,register char *argv[],char **e sh.bckpid = 0; sh.st.ioset=0; /* clean up any cooperating processes */ - if(sh.cpipe[0]>0) + if(sh.cpipe[0] > -1) sh_pclose(sh.cpipe); - if(sh.cpid && sh.outpipe) + if(sh.cpid && sh.outpipe && *sh.outpipe > -1) + { sh_close(*sh.outpipe); + *sh.outpipe = -1; + } sh.cpid = 0; if(sp=fcfile()) while(sfstack(sp,SF_POPSTACK)); diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c index 05dbdf2cd..92b2a78a7 100644 --- a/src/cmd/ksh93/sh/subshell.c +++ b/src/cmd/ksh93/sh/subshell.c @@ -455,8 +455,10 @@ void sh_subjobcheck(pid_t pid) { if(sp->cpid==pid) { - sh_close(sp->coutpipe); - sh_close(sp->cpipe); + if(sp->coutpipe > -1) + sh_close(sp->coutpipe); + if(sp->cpipe > -1) + sh_close(sp->cpipe); sp->coutpipe = sp->cpipe = -1; return; }