#4396 2>@stdout does 2>@1

[Lars Hellström]

In Unix. redirection of standard error of a child process to the stdout channel
doesn't redirect it to the stdout of the parent, but to the stdout of the child.
This means 2>@stdout behaves like 2>@1, which is not consistent with
the documentation of 2>@ redirection.

EXAMPLE

Consider first a trivial child which writes both to standard output and standard
error:

% catch {exec tclsh << {puts stdout OUT; puts stderr ERR; puts stdout OUT; puts stderr ERR; exit}} res
1
% set res
OUT
OUT
ERR
ERR

This is the usual "throw error if writing to stderr" behaviour of [exec]. In order
to work around it, one may redirect the child's standard error:

% catch {exec tclsh << {puts stdout OUT; puts stderr ERR; puts stdout OUT; puts stderr ERR; exit} 2>@ stderr} res
ERR
ERR
0
% set res
OUT
OUT

So far, all is well. My problem is however that I wanted to redirect a child's
standard error to the parent stdout, as I felt that would better reflect the actual
severity of the message that it could carry. To my surprise, this rather had
the effect of combining standard error with standard out:

% catch {exec tclsh << {puts stdout OUT; puts stderr ERR; puts stdout OUT; puts stderr ERR; exit} 2>@ stdout} res
0
% set res
OUT
ERR
OUT
ERR

This is the documented behaviour of 2>@1, but not of 2>@stdout, since 2>@
asks for the name of a channel in the parent, and stdout there is quite distinct
from the never-registered channel that [exec] internally reads its result from.

The bug appears to be in unix/tclUnixPipe.c, more precisely the
TclpCreateProcess function, and in particular with the code block

/*
* Set up stdio file handles for the child process.
*/

if (!SetupStdFile(inputFile, TCL_STDIN)
|| !SetupStdFile(outputFile, TCL_STDOUT)
|| (!joinThisError && !SetupStdFile(errorFile, TCL_STDERR))
|| (joinThisError &&
((dup2(1,2) == -1) || (fcntl(2, F_SETFD, 0) != 0)))) {
sprintf(errSpace,
"%dforked process couldn't set up input/output: ", errno);
write(fd, errSpace, (size_t) strlen(errSpace));
_exit(1);
}

As far as I can tell, what happens here in the case of an [exec ... 2>@ stdout]
is that errorFile points to file descriptor 1 whereas outputFile points to some
higher number file descriptor (T, say), and joinThisError is therefore false.
What goes wrong is that SetupStdFile(outputFile, TCL_STDOUT) performs a
dup2(T,1) before SetupStdFile(errorFile, TCL_STDERR) gets to do dup2(1,2),
and therefore both file descriptors 1 and 2 are made duplicates of file
descriptor T, rather than 2 being made a duplicate of the old 1 as the function
comments say it should. In pseudocode, we want to do

(fd[1],fd[2]) := (fd[T],fd[1]);

(parallel assignment) but currently serialize it as

fd[1] := fd[T]; fd[2] := fd[1];

which is not the same thing.

From this analogy with assignment -- that
(a,b) := (b,a);
needs at least three steps when serialized, e.g.
c:=a; a:=b; b:=c;
-- it follows that the only way to get this right in all cases is to preserve
the errorFile in a third file descriptor, so one needs to add a dup() call somehow,
but it is not obvious to me how to do this, since it appears a TclFile isn't quite
a file descriptor number, and SetupStdFile is sometimes creating the file
descriptor... Anyway, a test that it all works could be

exec tclsh << {
exec tclsh << {
puts stdout OUT1; puts stderr ERR1
puts stdout OUT2; puts stderr ERR2
exit
} >@stderr 2>@stdout
puts stdout ----
exit
}

(or something more robust to the same effect), which should have a return
code of 1 and a return value of
ERR1
ERR2
-----
OUT1
OUT2
since the middle tclsh switches places between stdout and stderr of the
innermost tclsh, so what it writes to its stderr would end up in the stdout
part of the result of the outer [exec]. In the present buggy state, it instead
comes out as
----
OUT1
ERR1
OUT2
ERR2
(inner stdout and stderr both redirected to middle stderr).

A slightly worrying factor is that upon rereading the 2>@1 TIP, I see that it
can be interpreted as stating that this behaviour of 2>@stdout as 2>@1
(though contrary to documentation) might have been known and used as
an idiom for 2>@1 before the latter was added. If that is so, then there may
be backwards compatibility issues, though certainly no larger than what we
have seen at minor version increments in the past.

[Alexandre Ferrieux]

Two strategies are possible:

(a) a brute-force dup() of those of in/out/errFile that are in [0,2], and passing the duped instead of the original to SetupStdChannel.

(b) a finer analysis of the dup2 graph to detect cases that need special attention.

(a) is trivial; (b) preserves the speed of the dominant, simple case.

Opinions ?

[Alexandre Ferrieux]

FWIW, (b) happens to be reducible to 3 tests:
- is stdin used in >@ ?
- is stdin used in 2>@ ?
- is stdout used in 2>@ ?
indeed only those cases need extra dupping. Attaching a patch doing just that. Please review. Untested (no unix at home) and lacking a bit of error checks after dup()s, but should be functional.

[Alexandre Ferrieux]

Untested method (b)

[Lars Hellström]

Fix of previous patch

[Lars Hellström]

dup.patch didn't quite work (it said fd2=GetFd(outputFile); and reused the "error" label), but I was able to work that out. Attaching fixed patch dup-2,patch. This also contains a new test (exec-15.8) covering 2>@stdout and >@stderr.

It might be notable that dup-2.patch also changes some error messages (those generated by the child after fork but before exec), but it seems no existing test covers these situations, so there are a couple of code paths here which are untested. Also, some of the (old) comments in that area seem a bit strange.

Finally, I've kept the closeOut and closeErr variables from dup.patch, but I'm not sure they're needed. Would not the extra file descriptors be closed at exec() anyway?

[Alexandre Ferrieux]

Ahem, that was untested code, right :-}
Thanks for fixing it Lars.
Regarding the explicit close() of the extra fds, you're probably right, I'm not in a position to check exactly their CLOEXEC status...
An additional concern is corner cases where 0,1,2 are not all opened, in which case dup() might land in [0,2], so a dup_over_3 function might be needed...