fix race condition in SCP for real.

We can't control a grand-child death through gzip, so stop spawning
grand-children: create two children connected by hand through a pipe,
and when we close gzip, explicitly wait for the second child to die
as well.

This avoids race conditions between sigpipe and sigusr1.

(and as usual, the resulting code is easier to follow once you get
through the pipe/fork).

Thanks theo for the comment. ;-)

2 files changed, 45 insertions, 77 deletions

diff --git a/usr.sbin/pkg_add/OpenBSD/PackageRepository.pm b/usr.sbin/pkg_add/OpenBSD/PackageRepository.pm
index c04ab7b5d57..870b1e197ed 100644
--- a/usr.sbin/pkg_add/OpenBSD/PackageRepository.pm
+++ b/usr.sbin/pkg_add/OpenBSD/PackageRepository.pm
@@ -1,5 +1,5 @@
 # ex:ts=8 sw=4:
-# $OpenBSD: PackageRepository.pm,v 1.9 2006/03/07 17:25:47 espie Exp $
+# $OpenBSD: PackageRepository.pm,v 1.10 2006/03/08 11:22:02 espie Exp $
 #
 # Copyright (c) 2003-2004 Marc Espie <espie@openbsd.org>
 #
@@ -87,6 +87,7 @@ sub close
 {
 	my ($self, $object, $hint) = @_;
 	close($object->{fh}) if defined $object->{fh};
+	waitpid($object->{pid2}, 0) if defined $object->{pid2};
 	$self->parse_problems($object->{errors}, $hint) 
 	    if defined $object->{errors};
 	undef $object->{errors};
@@ -366,6 +367,7 @@ sub pkg_copy
 	my $handler = sub {
 		my ($sig) = @_;
 		unlink $filename;
+		close($in);
 		$SIG{$sig} = 'DEFAULT';
 		kill $sig, $$;
 	};
@@ -407,6 +409,7 @@ sub pkg_copy
 	} else {
 		unlink $filename;
 	}
+	close($in);
 }
 
 sub open_pipe
@@ -417,48 +420,59 @@ sub open_pipe
 	$object->{errors} = OpenBSD::Temp::file();
 	$object->{cache_dir} = $ENV{'PKG_CACHE'};
 	$object->{parent} = $$;
+
+	my ($rdfh, $wrfh);
+	pipe($rdfh, $wrfh);
+
 	my $pid = open(my $fh, "-|");
 	if (!defined $pid) {
 		die "Cannot fork: $!";
 	}
 	if ($pid) {
 		$object->{pid} = $pid;
-		$object->{pid2} = <$fh>;
-		return $fh;
 	} else {
-		open STDERR, '>', $object->{errors};
-
-		my $pid2 = open(STDIN, "-|");
+		open(STDIN, '<&', $rdfh) or die "Bad dup";
+		close($rdfh);
+		close($wrfh);
+		exec {"/usr/bin/gzip"} 
+		    "gzip", 
+		    "-d", 
+		    "-c", 
+		    "-q", 
+		    "-" 
+		or die "can't run gzip";
+	}
+	my $pid2 = fork();
 
-		if (!defined $pid2) {
-			die "Cannot fork: $!";
-		}
-		if ($pid2) {
-			print $pid2, "\n";
-			exec {"/usr/bin/gzip"} 
-			    "gzip", 
-			    "-d", 
-			    "-c", 
-			    "-q", 
-			    "-" 
-			or die "can't run gzip";
-		} else {
-			if (defined $object->{cache_dir}) {
-				my $pid3 = open(my $in, "-|");
-				if (!defined $pid3) {
-					die "Cannot fork: $!";
-				}
-				if ($pid3) {
-					$self->pkg_copy($in, $object);
-					exit(0);
-				} else {
-					$self->grab_object($object);
-				}
+	if (!defined $pid2) {
+		die "Cannot fork: $!";
+	}
+	if ($pid2) {
+		$object->{pid2} = $pid2;
+	} else {
+		open STDERR, '>', $object->{errors};
+		open(STDOUT, '>&', $wrfh) or die "Bad dup";
+		close($rdfh);
+		close($wrfh);
+		close($fh);
+		if (defined $object->{cache_dir}) {
+			my $pid3 = open(my $in, "-|");
+			if (!defined $pid3) {
+				die "Cannot fork: $!";
+			}
+			if ($pid3) {
+				$self->pkg_copy($in, $object);
 			} else {
 				$self->grab_object($object);
 			}
+		} else {
+			$self->grab_object($object);
 		}
+		exit(0);
 	}
+	close($rdfh);
+	close($wrfh);
+	return $fh;
 }
 
 sub _list
diff --git a/usr.sbin/pkg_add/OpenBSD/PackageRepository/SCP.pm b/usr.sbin/pkg_add/OpenBSD/PackageRepository/SCP.pm
index febc41c95c4..4a76a3154f3 100644
--- a/usr.sbin/pkg_add/OpenBSD/PackageRepository/SCP.pm
+++ b/usr.sbin/pkg_add/OpenBSD/PackageRepository/SCP.pm
@@ -1,5 +1,5 @@
 # ex:ts=8 sw=4:
-# $OpenBSD: SCP.pm,v 1.5 2006/03/07 14:18:51 espie Exp $
+# $OpenBSD: SCP.pm,v 1.6 2006/03/08 11:22:02 espie Exp $
 #
 # Copyright (c) 2003-2004 Marc Espie <espie@openbsd.org>
 #
@@ -64,10 +64,6 @@ sub grab_object
 
 	my $cmdfh = $self->{cmdfh};
 	my $getfh = $self->{getfh};
-	$SIG{'USR1'} = sub {
-		kill USR1 => $object->{parent};
-		exit(1);
-	};
 
 	print $cmdfh "ABORT\n";
 	local $_;
@@ -103,22 +99,6 @@ sub grab_object
 	}
 }
 
-sub pkg_copy
-{
-	my ($self, $in, $object) = @_;
-
-	$SIG{'USR1'} = sub {
-		close($in);
-		if (defined $object->{tempname}) {
-			unlink $object->{tempname};
-		}
-		kill USR1 => $object->{parent};
-		exit(1);
-	};
-
-	$self->SUPER::pkg_copy($in, $object);
-}
-
 sub _new
 {
 	my ($class, $baseurl) = @_;
@@ -173,32 +153,6 @@ sub list
 	return $self->{list};
 }
 
-sub finish_and_close
-{
-	my ($self, $object) = @_;
-	$self->SUPER::close($object);
-}
-
-sub close
-{
-	my ($self, $object, $hint) = @_;
-	# XXX we have to make sure the grand-child is dead.
-	if (defined $object->{pid2}) {
-		my $received = 0;
-		local $SIG{'USR1'} = sub { $received = 1; };
-		kill USR1 => $object->{pid2};
-		while (!$received) {
-			sleep 0.01;
-		}
-	}
-	close($object->{fh}) if defined $object->{fh};
-	
-	$self->parse_problems($object->{errors}, $hint) 
-	    if defined $object->{errors};
-	undef $object->{errors};
-	$object->deref();
-}
-
 # XXX not used yet
 sub cleanup
 {