| Subject: | Mail::Mailer open failure w/taint in eval block causes nonintuive behavior |
I have a loop that for each entry, diddles with a database record, does
some external stuff, then sends an email, and writes back to the
database the date of sending. All this is wrapped in an eval for each
entry, and if any of the stuff fails or dies, it rolls back the database
transaction for that entry and dies. If it succeeds, it commits and
goes on to the next. (Fairly vanilla.)
Normally, I just tested that the MIME::Entity send method returns true,
but when I was running some tests under taint, I got a failure due to
insecure path that caused a die in my eval. For the first iteration of
the loop, this successfully rolled back the database transaction.
However, it then tried to commit! And it didn't bomb out of the overall
loop, either!
I spent a while tracking it down to a minimal test case that comes from
the fork and/or eval in Mail::Mailer->open. The code below demonstrates the problem on Debian testing w/ 5.8.4, and it is trying
to use sendmail; the key is to make sure that your path is tainted so
that the open call fails.
What happens is that the child from the open ("|-") dies before the exec
can kill it off, thereby causing the child's death to be caught in my
eval. The parent never discovers that the child's death happens.
I have a patch that seems to work for me with the sendmail version, but
I don't have a test suite for it. I'll send that under separate cover
shortly.
#!/usr/bin/perl -wT
use strict;
use Mail::Mailer;
for (1..3) {
eval {
my $m = new Mail::Mailer;
warn "Iteration $_, my PID $$";
$m->open( { To => 'rlucas', } ); # in real life, we would continue w/sending etc.
};
if ($@) {
warn "Original error for PID $$ was $@";
die "Died PID $$ trying to send email, here we would roll back a db transaction";
}
else {
warn "Here PID $$ would commit the db transaction";
}
}
--- Mailer.pm.bak 2005-05-19 19:18:17.000000000 -0700
+++ Mailer.pm 2005-05-19 19:22:49.000000000 -0700
@@ -266,9 +266,22 @@
$self->close; # just in case;
# Fork and start a mailer
- (defined($exe) && open($self,"|-"))
- || $self->exec($exe, $args, \@to)
- || die $!;
+ #(defined($exe) && open($self,"|-"))
+ #|| $self->exec($exe, $args, \@to)
+ #|| die $!;
+ if (my $childpid = open ($self, "|-") ){
+ # I am the parent
+ }
+ else {
+ # I am the child
+ eval {
+ $self->exec($exe, $args, \@to);
+ };
+ if ($@) {
+ warn "Forked child in " . __PACKAGE__ . " failed with $@";
+ exit(1);
+ }
+ }
# Set the headers
$self->set_headers($hdrs);
@@ -319,6 +332,7 @@
if (fileno($self)) {
$self->epilogue;
close($self)
+ or die "Could not close mailer: $!";
}
}