[2/2] test-terminal: drop stdin handling

Commit Message

Jeff King June 6, 2024, 8:22 a.m. UTC

Since 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
2015-08-04), we set up a pty and copy stdin to the child program. But
this ends up being racy; once we send all of the bytes and close the
descriptor, the child program will no longer see a terminal! isatty()
will return 0, and trying to read may return EIO, even if we didn't yet
get all of the bytes.

This was mentioned even in the commit message of 18d8c26930, but we
hacked around it by just sending an infinite input from /dev/zero (in
the intended case, we only cared about isatty(0), not reading actual
input).

And it came up again recently in:

  https://lore.kernel.org/git/[e-mail]/

where we tried to actually send bytes, but they don't always all come
through. So this interface is somewhat of an accident waiting to happen;
a caller might not even care about stdin being a tty, but will get bit
by the flaky behavior.

One solution would probably be to avoid closing test_terminal's end of
the pty altogether. But then the other side would never see EOF on its
stdin.  That may be OK for some cases, but it's another gotcha that
might cause races or deadlocks, depending on what the child expects to
read.

Let's instead just drop test_terminal's stdin feature completely. Since
the previous commit dropped the two cases from t4153 for which the
feature was originally added, there are no callers left that need it.

Signed-off-by: Jeff King <[e-mail]>
---
 t/test-terminal.perl | 29 +++--------------------------
 1 file changed, 3 insertions(+), 26 deletions(-)

Comments

Rubén Justo June 6, 2024, 4:14 p.m. UTC | #1

On 6/6/24 10:22 AM, Jeff King wrote:
> Since 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
> 2015-08-04), we set up a pty and copy stdin to the child program. But
> this ends up being racy; once we send all of the bytes and close the
> descriptor, the child program will no longer see a terminal! isatty()
> will return 0, and trying to read may return EIO, even if we didn't yet
> get all of the bytes.
> 
> This was mentioned even in the commit message of 18d8c26930, but we
> hacked around it by just sending an infinite input from /dev/zero (in
> the intended case, we only cared about isatty(0), not reading actual
> input).
> 
> And it came up again recently in:
> 
>   https://lore.kernel.org/git/[e-mail]/
> 
> where we tried to actually send bytes, but they don't always all come
> through. So this interface is somewhat of an accident waiting to happen;
> a caller might not even care about stdin being a tty, but will get bit
> by the flaky behavior.
> 
> One solution would probably be to avoid closing test_terminal's end of
> the pty altogether. But then the other side would never see EOF on its
> stdin.  That may be OK for some cases, but it's another gotcha that
> might cause races or deadlocks, depending on what the child expects to
> read.
> 
> Let's instead just drop test_terminal's stdin feature completely. Since
> the previous commit dropped the two cases from t4153 for which the
> feature was originally added, there are no callers left that need it.
> 
> Signed-off-by: Jeff King <[e-mail]>

Acked-by: Rubén Justo <[e-mail]>

Patch

diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 3810e9bb43..b8fd6a4f13 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -5,17 +5,15 @@ 
 use IO::Pty;
 use File::Copy;
 
-# Run @$argv in the background with stdio redirected to $in, $out and $err.
+# Run @$argv in the background with stdio redirected to $out and $err.
 sub start_child {
-	my ($argv, $in, $out, $err) = @_;
+	my ($argv, $out, $err) = @_;
 	my $pid = fork;
 	if (not defined $pid) {
 		die "fork failed: $!"
 	} elsif ($pid == 0) {
-		open STDIN, "<&", $in;
 		open STDOUT, ">&", $out;
 		open STDERR, ">&", $err;
-		close $in;
 		close $out;
 		exec(@$argv) or die "cannot exec '$argv->[0]': $!"
 	}
@@ -51,17 +49,6 @@  sub xsendfile {
 	copy($in, $out, 4096) or $!{EIO} or die "cannot copy from child: $!";
 }
 
-sub copy_stdin {
-	my ($in) = @_;
-	my $pid = fork;
-	if (!$pid) {
-		xsendfile($in, \*STDIN);
-		exit 0;
-	}
-	close($in);
-	return $pid;
-}
-
 sub copy_stdio {
 	my ($out, $err) = @_;
 	my $pid = fork;
@@ -81,25 +68,15 @@  sub copy_stdio {
 	die "usage: test-terminal program args";
 }
 $ENV{TERM} = 'vt100';
-my $parent_in = new IO::Pty;
 my $parent_out = new IO::Pty;
 my $parent_err = new IO::Pty;
-$parent_in->set_raw();
 $parent_out->set_raw();
 $parent_err->set_raw();
-$parent_in->slave->set_raw();
 $parent_out->slave->set_raw();
 $parent_err->slave->set_raw();
-my $pid = start_child(\@ARGV, $parent_in->slave, $parent_out->slave, $parent_err->slave);
-close $parent_in->slave;
+my $pid = start_child(\@ARGV, $parent_out->slave, $parent_err->slave);
 close $parent_out->slave;
 close $parent_err->slave;
-my $in_pid = copy_stdin($parent_in);
 copy_stdio($parent_out, $parent_err);
 my $ret = finish_child($pid);
-# If the child process terminates before our copy_stdin() process is able to
-# write all of its data to $parent_in, the copy_stdin() process could stall.
-# Send SIGTERM to it to ensure it terminates.
-kill 'TERM', $in_pid;
-finish_child($in_pid);
 exit($ret);