Skip Menu |
 

This queue is for tickets about the MailTools CPAN distribution.

Report information
The Basics
Id: 12890
Status: resolved
Priority: 0/
Queue: MailTools

People
Owner: Nobody in particular
Requestors: rlucas [...] tercent.com
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: 1.67
Fixed in: (no value)

Attachments


Subject: Mail::Mailer open failure w/taint in eval block causes nonintuive behavior
Download (untitled) / with headers
text/plain 1.8k
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: $!"; } }
Download (untitled) / with headers
text/plain 157b
The patch as submitted will exit(), which has some nonorthogonal effects on the parent as certain objects will have their DESTROY() methods called, it seems.
Download (untitled) / with headers
text/plain 843b
[guest - Tue May 24 12:54:03 2005]: Show quoted text
> The patch as submitted will exit(), which has some nonorthogonal effects > on the parent as certain objects will have their DESTROY() methods > called, it seems.
I only agree for 80% with your evaluation of the problem. eval() is certainly not the solution. What about this: (using my coding style) use POSIX qw/_exit/; ... my $child = open $self, '|-'; defined $child or die "Failed to send: $!" if($child==0) { # Child process will handle sending $self->exec($exe, $args, \@to); # should leave program warn $!; # exec failed _exit(1); # no DESTROY(), keep it for parent }
Download (untitled) / with headers
text/plain 773b
[MARKOV - Fri Jun 3 03:56:24 2005]: Show quoted text
> I only agree for 80% with your evaluation of the problem. eval() is > certainly not the solution.
... Show quoted text
> if($child==0) > { # Child process will handle sending > $self->exec($exe, $args, \@to); # should leave program > warn $!; # exec failed > _exit(1); # no DESTROY(), keep it for parent > }
The reason I wrapped the exec in an eval is that the self->exec call can die (e.g. under taint mode with an insecure path). If it dies, then in the above, you never get to _exit -- instead, the die propagates out of Mail::Mailer into the calling code (in the CHILD) and can trip an enclosing eval. There was a somewhat helpful PerlMonks thread on this at http://perlmonks.org/?node_id=459739
Download (untitled) / with headers
text/plain 995b
[guest - Fri Jun 3 15:20:49 2005]: Show quoted text
> > if($child==0) > > { # Child process will handle sending > > $self->exec($exe, $args, \@to); # should leave program > > warn $!; # exec failed > > _exit(1); # no DESTROY(), keep it for parent > > }
> > The reason I wrapped the exec in an eval is that the self->exec call can > die (e.g. under taint mode with an insecure path). If it dies, then in > the above, you never get to _exit -- instead, the die propagates out of > Mail::Mailer into the calling code (in the CHILD) and can trip an > enclosing eval.
Oh, I really hate that tied filehandle object idea.... it looks nice, but is a horror for treating errors correctly. So, are you satisfied with this attempt: if($child==0) { # Child process will handle sending eval { $self->exec($exe, $args, \@to) }; # should leave program warn $@ || $!; # exec failed _exit(1); # no DESTROY(), keep it for parent }
No reply, so fix added to next release. Ticket closed


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.