Re: Multio deadlock (Re: multios doesn't work with 2>&1)

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Oct 27,  8:33pm, Peter Stephenson wrote:
} Subject: Re: Multio deadlock (Re: multios doesn't work with 2>&1)
}
} >  	for (i = 0; i < 10; i++)
} > -	    if (fdtable[i] != FDT_UNUSED)
} > +	    if (i < 3 || fdtable[i] != FDT_UNUSED)
} >  		close(i);
} 
} Hmm... why don't we initialise fdtable[0..2] to FDT_INTERNAL at boot?

I don't know.  Is FDT_INTERNAL the correct flag?  It supposedly means
that those descriptors are "not visible to other processes" which does
not sound right to me.  I think they should be FDT_EXTERNAL by default?

} (FDT_UNUSED = 0, FDT_INTERNAL = 1).  However, I've alsoe looked later
} and seen all three set to FDT_INTERNAL; not sure where that happens but
} presumably something to do with redir.

It happens in utils.c:redup() but I'm not sure it's strictly correct.
FDT_INTERNAL means they'll be "closed on exec" in execcmd() which is
not right in the general case, but maybe it is when they've been dup'd.

} Shouldn't we just initialise them properly?

That would be better, yes, but they get toggled between FDT_UNUSED and
FDT_INTERNAL every time there's a redirection, first when the original
fd is closed [zclose()] and then when restored again afterward [redup()].

So the question is, in the deadlock case why is fd 2 open but still
marked FDT_UNUSED?  Who dup'd onto it without going through redup()?
Or is that not what happened?  (Its really difficult to trace this
with gdb because all of this happens in one of the child processes.)

See patch below, done essentially on a guess.

Interesting aside:  If fdtable[x] starts out FDT_EXTERNAL and is then
saved with movefd() and restored with redup(), its state changes to
FDT_INTERNAL because movefd() does not preserve the original state [it
marks everything internal], but redup() copies the state.

} We could in principle detect if they're open by attempting to dup() and
} seeing what fd we get.  Not sure if that's worth it, but it might be
} cleaner.

You mean, do that in order to determine whether they are already "unused"
when the shell starts up?  I don't think that's necessary.  If you mean
do it to avoid close(), it won't help, errno will still get frobbed.

The following does appear to prevent the deadlock, but probably is not
yet sufficient because it doesn't check that fdtable[] is big enough.


diff --git a/Src/exec.c b/Src/exec.c
index 99c7eaa..2e57443 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3124,6 +3124,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		    if(fd == -2)
 			fd = (fn->type == REDIR_MERGEOUT) ? coprocout : coprocin;
 		    fil = dup(fd);
+		    fdtable[fil] = FDT_INTERNAL;
 		}
 		if (fil == -1) {
 		    char fdstr[4];
@@ -3150,9 +3151,10 @@ execcmd(Estate state, int input, int output, int how, int last1)
 			       O_WRONLY | O_APPEND | O_CREAT | O_NOCTTY, 0666);
 		else
 		    fil = clobber_open(fn);
-		if(fil != -1 && IS_ERROR_REDIR(fn->type))
+		if(fil != -1 && IS_ERROR_REDIR(fn->type)) {
 		    dfil = dup(fil);
-		else
+		    fdtable[dfil] = FDT_INTERNAL;
+		} else
 		    dfil = 0;
 		if (fil == -1 || dfil == -1) {
 		    if(fil != -1)