MailScanner bug [Was: Re: SA 3.1.7 returning no result to MS?]

[David Lee]

On Wed, 15 Nov 2006, David Lee wrote:

> [...]
> Looking a little deeper, it seems that in the "MailScanner/SA.pm" module
> at the "eval { ...}" near line 800 (MS 4.56.8) all the results from:
>
>     $pipe->reader();
>     local $SIG{ALRM} = sub { die "Command Timed Out" };
>     alarm MailScanner::Config::Value('spamassassintimeout');
>     $SAHits = <$pipe>;
>     #print STDERR "Read SAHits = $SAHits " . scalar(localtime) . "\n";
>     $AutoLearn = <$pipe>;
>     $SAHitList = <$pipe>;
>     $SAReport  = <$pipe>;
>     #print STDERR "Read SAHitList = $SAHitList " . scalar(localtime) . "\n";
>     # Not sure if next 2 lines should be this way round...
>     waitpid $pid, 0;
>     $pipe->close();
>     $PipeReturn = $?;
>
> are coming back empty.  Hence the syslog entry:
>    Message ... is not spam, SpamAssassin (cached, score=0, required 6, autolearn=)
>
> This suggests that SA's expected behaviour would be that a 'good' return
> would always be accompanied by real, non-empty, data.  This, I presume, is
> what has always happened historically.
>
> But it now seems, after upgrade to SA 3.1.7, that SA occasionally provides
> empty data on a 'good' return: inconsistent.


I've probably slightly mis-read the detail there.  But the same principle
still applies.  I think we have an MS bug.

The "MailScanner/SA.pm" code (my comments "##TDL##") does:


  my $pid = fork();
  [...]

  if ($pid == 0) {
    # In the child              ##TDL## Child does the SpamAssassin call
    $pipe->writer();
    [...]
      $spamness = $Test->check($mail);
    [ ... print SA results into $pipe ...]
    $pipe->close();
    exit 0; # $SAResult;
  }
                                ##TDL## Note: child has finished.
                                ##TDL## In MailScanner parent only.
  eval {
    $pipe->reader();
    [... read SA results ...]   ##TDL## from the SA call in child above.
    $pipe->close();
    $PipeReturn = $?;
    [...]
  }
                                ##TDL## "$PipeReturn" never used


Now suppose that SA (from the fork's child) crashes.  Ideally, this
"shouldn't happen"(TM).  But we all know reality to be non-ideal, don't
we?  And in my own environment, from a different context, I have seen such
SA-3.1.7 crashes actually happen.  (If anyone is curious, I can provide
further info and core files.)

Then the parent (i.e. inside that "eval {}") will get empty data from SA,
and the parent will continue to run.  Worse, the existing code of MS fails
to distinguish whether the SA/child terminated cleanly (with real data
from SA/child/pipe) or crashed (no data from SA/child/pipe).

The parent stores a return code in "$PipeReturn".  Would that particular
code distinguish the crash from the normal termination?  Even if it does
potentially distinguish the various return types (success, fail, crash) of
the SA/child, the subsequent code doesn't ever use it (except merely
printing it as a low-priority debugging message 100 lines later).

I suggest therefore, that there is a MailScanner bug:



MS BUG: If MS's call to SA fails (e.g. crash/segfault), MS is unable to
distinguish this from a successful return.

RESOLUTION: The code immediately following the "eval {}" ought to check
the return (e.g "$PipeReturn") to detect SA/child failures.  Such failures
may reaonably be expected to be rare, but should nevertheless be handled,
perhaps by leaving the email where it is, and doing a high-priority (e.g.
"error") syslog message.

NOTE: Might the SA/child itself need an enclosing "eval {}"?






-- 

:  David Lee                                I.T. Service          :
:  Senior Systems Programmer                Computer Centre       :
:                                           Durham University     :
:  http://www.dur.ac.uk/t.d.lee/            South Road            :
:                                           Durham DH1 3LE        :
:  Phone: +44 191 334 2752                  U.K.                  :