| Subject: | child fd inadvertently closed if TFDs need to be moved |
If the parent process has an open fd which is the first in a sequence
of fds that will be used by the child process, the child fd accounting
code loses track of which fds to close, and ends up closing one needed
by the eventual sub-process.
The attached bugtest.pl code illustrates this. It should run without
error if everything works, otherwise it will complain about not being
able to open an fd.
Here's an explanation of the problem; bugtest.pl essentially sets up
the environment so that this happens.
Assume that the parent has fd=3 open. The child requires fd=3 & fd=4.
The parent opens fds 4 and 5 to handle the child's fds, as
it can't perform a direct map because of fd=3 being open.
The parent creates OPS entries with
OPS[0]: { TFD => 4, KFD => 3 }
OPS[1]: { TFD => 5, KFD => 4 }
In the child, fd=3 is closed as it is not needed.
The child handles the moving of fds in _do_kid_and_exit:
2597 ## Lazy closing is so the same fd (ie the same TFD value) can be dup2'ed on
2598 ## several times.
2600 for ( @{$kid->{OPS}} ) {
2601 if ( defined $_->{TFD} ) {
2602 unless ( $_->{TFD} == $_->{KFD} ) {
2603 $self->_dup2_gently( $kid->{OPS}, $_->{TFD}, $_->{KFD} );
2604 push @lazy_close, $_->{TFD};
2605 }
2606 }
....
2623 }
When servicing OPS[0], _dup2_gently dups fd=4 to fd=3 and line 2604
records fd=4 as needing to be closed.
When servicing OPS[1], _dup2_gently notices that OPS[0]{TFD} conflicts
with OPS[1]{KFD} and dups OPS[0]{TFD} elsewhere. It then dups fd=5
to fd=4 and line 2604 records fd=5 as needing to be closed.
At this point the fds marked in @lazy_close are closed:
2625 for ( @lazy_close ) {
2626 unless ( $closed[$_] ) {
2627 _close( $_ );
2628 $closed[$_] = 1;
2629 }
2630 }
and fd=4 gets closed even though it is needed by OPS[1].
So that's one bug. The other bug is that @lazy_close doesn't know
that OPS[0]{TFD} got moved (that was done in _dup2_gently), so that
fd does not get closed.
There's a quick and dirty work-around to fix the first problem, but I
think there is a more substantial problem at work here. There are
multiple different places that information about fds are kept:
@fds
@needed
@closed
@lazy_close
I think this can be simplified, and have attached a patch agains version 0.92 to
do so. The following notes are for the child process only; I haven't
analyzed the code for the parent process. The patch doesn't affect the
parent process.
* @fds is the most important data structure; it records the known open
fds. It is already consistently managed by the _close, _dup, and
_dup2_rudely routines.
* @needed is useful, but because it is separate from @fds, it can
get out of sync. it is also local to a single function (_do_kid_and_exit)
when it needs to be accessible elsewhere (e.g. _dup2_gently)
* There's no need for @closed; if the fd is not in @fds, it's not open.
* @lazy_close means that an fd is not needed, but don't delete
it right away. There are two aspects to this. First, there's
already the @needed array, so this duplicates that functionality.
Second, @lazy_close is never used as a guard to *prevent* closing an
fd. So it really has no use. Simply recording the fd in @needed as
not being needed is sufficient.
My patch does a couple of things.
* It uses @fds as the *only* repository of information about
fds, so all information is centralized. @fds is now an array of
hashes.
* @needed is replaced by the {needed} attribute in an @fds element,
e.g. fds[0]{needed} = 1.
* Since @fds is the canonical list of fds and is managed correctly,
@closed is not needed.
* @lazy_close is also no longer needed; the {needed} attribute for an
fd is set to 0 so that it will be closed during the next sweep.
* the code is more explicit about specifying which fds are needed or
not. In particular, it explicitly marks KFDs as needed after they
are created. This prevents inadvertent closing of KFD's whose fds
were previously TFDs. When a TFD fd is moved by _dup2_gently the new
entry in @fds defaults to {needed} = 0, and it will get cleaned up
during the next fd closing sweep.
I could not come up with a clean way of adding a test; the attached
bugtest.pl is designed to be easily dropped into the test suite with
a few minor tweaks.
of fds that will be used by the child process, the child fd accounting
code loses track of which fds to close, and ends up closing one needed
by the eventual sub-process.
The attached bugtest.pl code illustrates this. It should run without
error if everything works, otherwise it will complain about not being
able to open an fd.
Here's an explanation of the problem; bugtest.pl essentially sets up
the environment so that this happens.
Assume that the parent has fd=3 open. The child requires fd=3 & fd=4.
The parent opens fds 4 and 5 to handle the child's fds, as
it can't perform a direct map because of fd=3 being open.
The parent creates OPS entries with
OPS[0]: { TFD => 4, KFD => 3 }
OPS[1]: { TFD => 5, KFD => 4 }
In the child, fd=3 is closed as it is not needed.
The child handles the moving of fds in _do_kid_and_exit:
2597 ## Lazy closing is so the same fd (ie the same TFD value) can be dup2'ed on
2598 ## several times.
2600 for ( @{$kid->{OPS}} ) {
2601 if ( defined $_->{TFD} ) {
2602 unless ( $_->{TFD} == $_->{KFD} ) {
2603 $self->_dup2_gently( $kid->{OPS}, $_->{TFD}, $_->{KFD} );
2604 push @lazy_close, $_->{TFD};
2605 }
2606 }
....
2623 }
When servicing OPS[0], _dup2_gently dups fd=4 to fd=3 and line 2604
records fd=4 as needing to be closed.
When servicing OPS[1], _dup2_gently notices that OPS[0]{TFD} conflicts
with OPS[1]{KFD} and dups OPS[0]{TFD} elsewhere. It then dups fd=5
to fd=4 and line 2604 records fd=5 as needing to be closed.
At this point the fds marked in @lazy_close are closed:
2625 for ( @lazy_close ) {
2626 unless ( $closed[$_] ) {
2627 _close( $_ );
2628 $closed[$_] = 1;
2629 }
2630 }
and fd=4 gets closed even though it is needed by OPS[1].
So that's one bug. The other bug is that @lazy_close doesn't know
that OPS[0]{TFD} got moved (that was done in _dup2_gently), so that
fd does not get closed.
There's a quick and dirty work-around to fix the first problem, but I
think there is a more substantial problem at work here. There are
multiple different places that information about fds are kept:
@fds
@needed
@closed
@lazy_close
I think this can be simplified, and have attached a patch agains version 0.92 to
do so. The following notes are for the child process only; I haven't
analyzed the code for the parent process. The patch doesn't affect the
parent process.
* @fds is the most important data structure; it records the known open
fds. It is already consistently managed by the _close, _dup, and
_dup2_rudely routines.
* @needed is useful, but because it is separate from @fds, it can
get out of sync. it is also local to a single function (_do_kid_and_exit)
when it needs to be accessible elsewhere (e.g. _dup2_gently)
* There's no need for @closed; if the fd is not in @fds, it's not open.
* @lazy_close means that an fd is not needed, but don't delete
it right away. There are two aspects to this. First, there's
already the @needed array, so this duplicates that functionality.
Second, @lazy_close is never used as a guard to *prevent* closing an
fd. So it really has no use. Simply recording the fd in @needed as
not being needed is sufficient.
My patch does a couple of things.
* It uses @fds as the *only* repository of information about
fds, so all information is centralized. @fds is now an array of
hashes.
* @needed is replaced by the {needed} attribute in an @fds element,
e.g. fds[0]{needed} = 1.
* Since @fds is the canonical list of fds and is managed correctly,
@closed is not needed.
* @lazy_close is also no longer needed; the {needed} attribute for an
fd is set to 0 so that it will be closed during the next sweep.
* the code is more explicit about specifying which fds are needed or
not. In particular, it explicitly marks KFDs as needed after they
are created. This prevents inadvertent closing of KFD's whose fds
were previously TFDs. When a TFD fd is moved by _dup2_gently the new
entry in @fds defaults to {needed} = 0, and it will get cleaned up
during the next fd closing sweep.
I could not come up with a clean way of adding a test; the attached
bugtest.pl is designed to be easily dropped into the test suite with
a few minor tweaks.
| Subject: | bugtest.pl |
use IPC::Run 'run';
use File::Temp;
use IO::Handle;
use POSIX ();
# trigger IPC::Run bug where parent has $fd open
# and child needs $fd & $fd+1
sub parent {
# dup stderr so we get some fd
my $xfd = POSIX::dup( 2 );
die $! if $xfd == -1;
my @fds = ( $xfd, $xfd + 1 );
# create input files to be attached to the fds
my @tmp;
@tmp[@fds] = map {
my $tmp = File::Temp->new;
$tmp->print( $_ );
$tmp->close;
$tmp
} @fds;
# child reads from fds and make sure that
# it can open them and that they're attached
# to the files it expects.
my $child = sub {
for my $fd ( @fds ) {
my $io = IO::Handle->new_from_fd( $fd, '<' )
or die( "error fdopening $fd\n" );
my $input = $io->getline;
die "expected >$fd<. got >$input<\n"
unless $fd eq $input;
}
};
run $child, map { $_ . '<', $tmp[$_]->filename } @fds;
POSIX::close $xfd;
}
parent;
| Subject: | IPC-Run-0.92.patch |
# This is a patch for IPC-Run-0.92.orig to update it to IPC-Run-0.92
#
# To apply this patch:
# STEP 1: Chdir to the source directory.
# STEP 2: Run the 'applypatch' program with this patch file as input.
#
# If you do not have 'applypatch', it is part of the 'makepatch' package
# that you can fetch from the Comprehensive Perl Archive Network:
# http://www.perl.com/CPAN/authors/Johan_Vromans/makepatch-x.y.tar.gz
# In the above URL, 'x' should be 2 or higher.
#
# To apply this patch without the use of 'applypatch':
# STEP 1: Chdir to the source directory.
# STEP 2: Run the 'patch' program with this file as input.
#
#### End of Preamble ####
#### Patch data follows ####
diff -c 'IPC-Run-0.92.orig/lib/IPC/Run.pm' 'IPC-Run-0.92/lib/IPC/Run.pm'
Index: ./lib/IPC/Run.pm
*** ./lib/IPC/Run.pm Thu Aug 30 11:22:46 2012
--- ./lib/IPC/Run.pm Sun Dec 9 15:15:31 2012
***************
*** 1248,1254 ****
croak "$!: dup( $_[0] )" unless defined $r;
$r = 0 if $r eq '0 but true';
_debug "dup( $_[0] ) = $r" if _debugging_details;
! $fds{$r} = 1;
return $r;
}
--- 1248,1254 ----
croak "$!: dup( $_[0] )" unless defined $r;
$r = 0 if $r eq '0 but true';
_debug "dup( $_[0] ) = $r" if _debugging_details;
! $fds{$r} = {};
return $r;
}
***************
*** 1259,1265 ****
croak "$!: dup2( $_[0], $_[1] )" unless defined $r;
$r = 0 if $r eq '0 but true';
_debug "dup2( $_[0], $_[1] ) = $r" if _debugging_details;
! $fds{$r} = 1;
return $r;
}
--- 1259,1265 ----
croak "$!: dup2( $_[0], $_[1] )" unless defined $r;
$r = 0 if $r eq '0 but true';
_debug "dup2( $_[0], $_[1] ) = $r" if _debugging_details;
! $fds{$r} = {};
return $r;
}
***************
*** 1301,1307 ****
croak "$!: open( $_[0], ", sprintf( "0x%03x", $_[1] ), " )" unless defined $r;
_debug "open( $_[0], ", sprintf( "0x%03x", $_[1] ), " ) = $r"
if _debugging_data;
! $fds{$r} = 1;
return $r;
}
--- 1301,1307 ----
croak "$!: open( $_[0], ", sprintf( "0x%03x", $_[1] ), " )" unless defined $r;
_debug "open( $_[0], ", sprintf( "0x%03x", $_[1] ), " ) = $r"
if _debugging_data;
! $fds{$r} = {};
return $r;
}
***************
*** 1312,1318 ****
my ( $r, $w ) = POSIX::pipe;
croak "$!: pipe()" unless defined $r;
_debug "pipe() = ( $r, $w ) " if _debugging_details;
! $fds{$r} = $fds{$w} = 1;
return ( $r, $w );
}
--- 1312,1318 ----
my ( $r, $w ) = POSIX::pipe;
croak "$!: pipe()" unless defined $r;
_debug "pipe() = ( $r, $w ) " if _debugging_details;
! ( $fds{$r}, $fds{$w}) = ( {}, {} );
return ( $r, $w );
}
***************
*** 1346,1352 ****
$pty->blocking( 0 ) or croak "$!: pty->blocking ( 0 )";
_debug "pty() = ( ", $pty->fileno, ", ", $pty->slave->fileno, " )"
if _debugging_details;
! $fds{$pty->fileno} = $fds{$pty->slave->fileno} = 1;
return $pty;
}
--- 1346,1352 ----
$pty->blocking( 0 ) or croak "$!: pty->blocking ( 0 )";
_debug "pty() = ( ", $pty->fileno, ", ", $pty->slave->fileno, " )"
if _debugging_details;
! ( $fds{$pty->fileno}, $fds{$pty->slave->fileno} ) = ( {}, {} );
return $pty;
}
***************
*** 2487,2495 ****
next unless defined $_->{TFD};
$_->{TFD} = _dup( $_->{TFD} ) if $_->{TFD} == $fd2;
}
! $self->{DEBUG_FD} = _dup $self->{DEBUG_FD}
! if defined $self->{DEBUG_FD} && $self->{DEBUG_FD} == $fd2;
!
_dup2_rudely( $fd1, $fd2 );
}
--- 2487,2496 ----
next unless defined $_->{TFD};
$_->{TFD} = _dup( $_->{TFD} ) if $_->{TFD} == $fd2;
}
! if ( defined $self->{DEBUG_FD} && $self->{DEBUG_FD} == $fd2 ) {
! $self->{DEBUG_FD} = _dup $self->{DEBUG_FD};
! $fds{$self->{DEBUG_FD}}{needed} = 1;
! }
_dup2_rudely( $fd1, $fd2 );
}
***************
*** 2540,2577 ****
## close parent FD's first so they're out of the way.
## Don't close STDIN, STDOUT, STDERR: they should be inherited or
## overwritten below.
! my @needed = $self->{noinherit} ? () : ( 1, 1, 1 );
! $needed[ $self->{SYNC_WRITER_FD} ] = 1;
! $needed[ $self->{DEBUG_FD} ] = 1 if defined $self->{DEBUG_FD};
! for ( @{$kid->{OPS}} ) {
! $needed[ $_->{TFD} ] = 1 if defined $_->{TFD};
! }
## TODO: use the forthcoming IO::Pty to close the terminal and
## make the first pty for this child the controlling terminal.
## This will also make it so that pty-laden kids don't cause
## other kids to lose stdin/stdout/stderr.
! my @closed;
if ( %{$self->{PTYS}} ) {
## Clean up the parent's fds.
for ( keys %{$self->{PTYS}} ) {
_debug "Cleaning up parent's ptty '$_'" if _debugging_details;
my $slave = $self->{PTYS}->{$_}->slave;
! $closed[ $self->{PTYS}->{$_}->fileno ] = 1;
close $self->{PTYS}->{$_};
$self->{PTYS}->{$_} = $slave;
}
close_terminal;
! $closed[ $_ ] = 1 for ( 0..2 );
}
for my $sibling ( @{$self->{KIDS}} ) {
for ( @{$sibling->{OPS}} ) {
if ( $_->{TYPE} =~ /^.pty.$/ ) {
$_->{TFD} = $self->{PTYS}->{$_->{PTY_ID}}->fileno;
! $needed[$_->{TFD}] = 1;
}
# for ( $_->{FD}, ( $sibling != $kid ? $_->{TFD} : () ) ) {
--- 2541,2579 ----
## close parent FD's first so they're out of the way.
## Don't close STDIN, STDOUT, STDERR: they should be inherited or
## overwritten below.
! do { $_->{needed} = 1 for @fds{0..2} }
! unless $self->{noinherit};
! $fds{$self->{SYNC_WRITER_FD}}{needed} = 1;
! $fds{$self->{DEBUG_FD}}{needed} = 1 if defined $self->{DEBUG_FD};
!
! $fds{$_->{TFD}}{needed} = 1
! foreach grep { defined $_->{TFD} } @{$kid->{OPS} };
## TODO: use the forthcoming IO::Pty to close the terminal and
## make the first pty for this child the controlling terminal.
## This will also make it so that pty-laden kids don't cause
## other kids to lose stdin/stdout/stderr.
!
if ( %{$self->{PTYS}} ) {
## Clean up the parent's fds.
for ( keys %{$self->{PTYS}} ) {
_debug "Cleaning up parent's ptty '$_'" if _debugging_details;
my $slave = $self->{PTYS}->{$_}->slave;
! delete $fds{$self->{PTYS}->{$_}->fileno};
close $self->{PTYS}->{$_};
$self->{PTYS}->{$_} = $slave;
}
close_terminal;
! delete @{fds}{0..2};
}
for my $sibling ( @{$self->{KIDS}} ) {
for ( @{$sibling->{OPS}} ) {
if ( $_->{TYPE} =~ /^.pty.$/ ) {
$_->{TFD} = $self->{PTYS}->{$_->{PTY_ID}}->fileno;
! $fds{$_->{TFD}}{needed} = 1;
}
# for ( $_->{FD}, ( $sibling != $kid ? $_->{TFD} : () ) ) {
***************
*** 2587,2618 ****
## This is crude: we have no way of keeping track of browsing all open
## fds, so we scan to a fairly high fd.
_debug "open fds: ", join " ", keys %fds if _debugging_details;
- for (keys %fds) {
- if ( ! $closed[$_] && ! $needed[$_] ) {
- _close( $_ );
- $closed[$_] = 1;
- }
- }
! ## Lazy closing is so the same fd (ie the same TFD value) can be dup2'ed on
! ## several times.
! my @lazy_close;
for ( @{$kid->{OPS}} ) {
if ( defined $_->{TFD} ) {
unless ( $_->{TFD} == $_->{KFD} ) {
$self->_dup2_gently( $kid->{OPS}, $_->{TFD}, $_->{KFD} );
! push @lazy_close, $_->{TFD};
}
}
elsif ( $_->{TYPE} eq 'dup' ) {
$self->_dup2_gently( $kid->{OPS}, $_->{KFD1}, $_->{KFD2} )
unless $_->{KFD1} == $_->{KFD2};
}
elsif ( $_->{TYPE} eq 'close' ) {
for ( $_->{KFD} ) {
! if ( ! $closed[$_] ) {
_close( $_ );
- $closed[$_] = 1;
$_ = undef;
}
}
--- 2589,2615 ----
## This is crude: we have no way of keeping track of browsing all open
## fds, so we scan to a fairly high fd.
_debug "open fds: ", join " ", keys %fds if _debugging_details;
! _close( $_ ) foreach grep { ! $fds{$_}{needed} } keys %fds;
!
for ( @{$kid->{OPS}} ) {
if ( defined $_->{TFD} ) {
unless ( $_->{TFD} == $_->{KFD} ) {
$self->_dup2_gently( $kid->{OPS}, $_->{TFD}, $_->{KFD} );
! $fds{$_->{TFD}}{needed} = 0;
! $fds{$_->{KFD}}{needed} = 1;
}
}
elsif ( $_->{TYPE} eq 'dup' ) {
$self->_dup2_gently( $kid->{OPS}, $_->{KFD1}, $_->{KFD2} )
unless $_->{KFD1} == $_->{KFD2};
+ $fds{$_->{KFD2}}{needed} = 1;
+
}
elsif ( $_->{TYPE} eq 'close' ) {
for ( $_->{KFD} ) {
! if ( $fds{$_ } ) {
_close( $_ );
$_ = undef;
}
}
***************
*** 2622,2633 ****
}
}
! for ( @lazy_close ) {
! unless ( $closed[$_] ) {
! _close( $_ );
! $closed[$_] = 1;
! }
! }
if ( ref $kid->{VAL} ne 'CODE' ) {
open $s1, ">&=$self->{SYNC_WRITER_FD}"
--- 2619,2625 ----
}
}
! _close( $_ ) foreach grep { ! $fds{$_}{needed} } keys %fds;
if ( ref $kid->{VAL} ne 'CODE' ) {
open $s1, ">&=$self->{SYNC_WRITER_FD}"
#### End of Patch data ####
#### ApplyPatch data follows ####
# Data version : 1.0
# Date generated : Sun Dec 9 15:44:22 2012
# Generated by : makepatch 2.05
# Recurse directories : Yes
# Excluded files : (\A|/).*\~\Z
# (\A|/).*\.a\Z
# (\A|/).*\.bak\Z
# (\A|/).*\.BAK\Z
# (\A|/).*\.elc\Z
# (\A|/).*\.exe\Z
# (\A|/).*\.gz\Z
# (\A|/).*\.ln\Z
# (\A|/).*\.o\Z
# (\A|/).*\.obj\Z
# (\A|/).*\.olb\Z
# (\A|/).*\.old\Z
# (\A|/).*\.orig\Z
# (\A|/).*\.rej\Z
# (\A|/).*\.so\Z
# (\A|/).*\.Z\Z
# (\A|/)\.del\-.*\Z
# (\A|/)\.make\.state\Z
# (\A|/)\.nse_depinfo\Z
# (\A|/)core\Z
# (\A|/)tags\Z
# (\A|/)TAGS\Z
# p 'lib/IPC/Run.pm' 137888 1355084131 0100644
#### End of ApplyPatch data ####
#### End of Patch kit [created: Sun Dec 9 15:44:22 2012] ####
#### Patch checksum: 309 9852 1368 ####
#### Checksum: 327 10542 58242 ####