| Subject: | Inconsistency in send_by_sendmail causes problems |
From 3.01:
sub send_by_sendmail {
my $self = shift;
if (@_ == 1) { ### Use the given command...
my $sendmailcmd = shift @_;
### Do it:
open SENDMAIL, "|$sendmailcmd" or Carp::croak "open
|$sendmailcmd: $!\n";
$self->print(\*SENDMAIL);
close SENDMAIL;
return (($? >> 8) ? undef : 1);
}
else { ### Build the command...
my %p = @_;
$p{Sendmail} ||= "/usr/lib/sendmail";
### Start with the command and basic args:
my @cmd = ($p{Sendmail}, @{$p{BaseArgs} || ['-t', '-oi', '-oem']});
### See if we are forcibly setting the sender:
$p{SetSender} = 1 if defined($p{FromSender});
### Add the -f argument, unless we're explicitly told NOT to:
unless (exists($p{SetSender}) and !$p{SetSender}) {
my $from = $p{FromSender} || ($self->get('From'))[0];
if ($from) {
my ($from_addr) = extract_addrs($from);
push @cmd, "-f$from_addr" if $from_addr;
}
}
### Open the command in a taint-safe fashion:
my $pid = open SENDMAIL, "|-";
defined($pid) or die "open of pipe failed: $!\n";
if (!$pid) { ### child
exec(@cmd) or die "can't exec $p{Sendmail}: $!\n";
### NOTREACHED
}
else { ### parent
$self->print(\*SENDMAIL);
close SENDMAIL || die "error closing $p{Sendmail}: $! (exit
$?)\n";
return 1;
}
}
}
There are two inconsistencies that cause problems. The first one ties to
bug #2759; because the argument construction is only done if arguments
are not passed in, then it means that if arguments are passed in,
SetSender is ignored, which is contrary to the documentation. Recommend
a documentation fix to address this issue.
The second inconsistency that can cause problems, is that sendmail's
pipe is opened two different ways, and in one case returns an error
while in the other case it may die. In one case, it appears to open a
pipe to the command, and then it prints into the pipe closes and
returns. In the second case, it forks a pipe, executes sendmail in the
child, and may die if there is an error, otherwise it always returns
success.
It is my belief that the two methods should be identical: either both
die, or both return errors on failure. I don't have an opinion on
whether or not one of them should fork or not. I believe the fix should
be to make both return errors when appropriate -- there is (I would
imagine) a fairly large legacy codebase, which was developed just using
send() and letting it choose correct defaults. Changing the default
behaviour to die() could cause these codebases to break in bad ways.