Bug 4997 - [review] sa-update triggers errors in IO/Zlib.pm
Summary: [review] sa-update triggers errors in IO/Zlib.pm
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: sa-update (show other bugs)
Version: 3.1.3
Hardware: Other Linux
: P5 normal
Target Milestone: 3.1.4
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard: can be commited
Keywords:
Depends on: 4941
Blocks:
  Show dependency tree
 
Reported: 2006-07-19 08:42 UTC by Igmar Palsenberg
Modified: 2006-07-21 08:02 UTC (History)
0 users



Attachment Type Modified Status Actions Submitter/CLA Status
suggested patch patch None Theo Van Dinter [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Igmar Palsenberg 2006-07-19 08:42:06 UTC
sa-update bails out with a :

[22930] dbg: channel: file verification passed, installing update
[22930] dbg: channel: cleaning out update directory
Not a GLOB reference at /usr/lib/perl5/vendor_perl/5.8.5/IO/Zlib.pm line 566.

which leaves spamd with an empty rule set :)

Relevant info : 

CentOS 4.3
perl 5.8.5-24 (standard)
perl-Compress-Zlib 1.42 (home cooked)
perl-IO-Zlib 1.04 (home cooked)

Both of them pass the 'make test' suite. Both SA and amavisd seem to run fine,
it's only sa-update giving problems.

Full debug session (sa-update -D) is available at
http://www.palsenberg.com/plain/sa-update.dbg
Comment 1 Justin Mason 2006-07-19 09:45:59 UTC
based on the logs (thanks for the full debug log!) it looks like it died at the
IO::Zlib constructor:

---------
  dbg("channel: cleaning out update directory");
  if (!clean_update_dir($UPDTmp)) {
    channel_failed("channel: attempt to clean update dir failed");
    next;
  }

  unlink $CFFTmp || warn "error: can't remove file $CFFTmp: $!\n";

  $tfh = IO::Zlib->new($content_file, "rb");  <<<<DIED HERE
  die "fatal: couldn't read content tmpfile $content_file: $!\n" unless $tfh;

  my $tar = Archive::Tar->new($tfh);
  die "fatal: couldn't create Archive::Tar object!\n" unless $tar;

  dbg("channel: extracting archive");
---------


IO::Zlib line 566, for me, is:

    return tied(*{$self})->$AUTOLOAD(@_);

blargh, what a mess ;)   I think that implies that $self was invalid.

I can't see a reason why this would happen, but clearly it did.
we need to use eval { } around the IO::Zlib calls to trap this...
I suppose assuming that it wouldn't throw random die()s around
was a little naive :(
Comment 2 Justin Mason 2006-07-19 09:46:51 UTC
aiming at 3.1.5; perhaps it needs to be fixed for 3.1.4 though?  sa-update dying
and leaving an empty ruleset is a very major bug.
Comment 3 Igmar Palsenberg 2006-07-19 10:41:28 UTC
(In reply to comment #1)
> IO::Zlib line 566, for me, is:
> 
>     return tied(*{$self})->$AUTOLOAD(@_);
> 
> blargh, what a mess ;)   I think that implies that $self was invalid.

Confirmed, the code is exactly the same here. My last Perl hackery was 6 years
ago, so that makes it hard for me to actually fix this. I'm starting to think
that the perl on this machine has bugs, I'll see if I can upgrade that to 5.8.8,
and see what it does.
Comment 4 Theo Van Dinter 2006-07-19 14:37:51 UTC
This came up before, and iirc it ended up being a module that was out-of-date on
the user's machine.  I'll see if I can dig up the thread.
Comment 5 Theo Van Dinter 2006-07-19 14:46:33 UTC
(In reply to comment #4)
> This came up before, and iirc it ended up being a module that was out-of-date on
> the user's machine.  I'll see if I can dig up the thread.

Aha:

http://www.nabble.com/Re%3A-3.1.1-Upgrade-Problems-tf1301352.html#a3468275

the solution was to upgrade Archive::Tar to a newer version.  I have 1.23
installed and things work fine, the person in the thread had 1.00 and upgraded
to 1.29 which fixed the issue.

We should probably require a minimum version of both IO::Zlib and Archive::Tar.
Comment 6 Igmar Palsenberg 2006-07-19 14:51:51 UTC
> the solution was to upgrade Archive::Tar to a newer version.  I have 1.23
> installed and things work fine, the person in the thread had 1.00 and upgraded
> to 1.29 which fixed the issue.
> 
> We should probably require a minimum version of both IO::Zlib and Archive::Tar.

Indeed, that fixed it for me too. I made a list of all modules it used, and
check if the version of the module was current according to CPAN.

Best way is indeed check the version, and bail out if it doesn't provide a
certain version. I think this should be fixed in the sa-update and, if needed,
in the SpamAssassin code.
Comment 7 Justin Mason 2006-07-19 14:57:53 UTC
IMO, however, we should *also* use eval { } to trap die()s from those modules,
so that they cannot leave the system with an empty ruleset.  that's a serious
bug....

essentially, we should be in a defensive stance against bugs in those modules.
Comment 8 Theo Van Dinter 2006-07-19 15:27:10 UTC
(In reply to comment #7)
> IMO, however, we should *also* use eval { } to trap die()s from those modules,
> so that they cannot leave the system with an empty ruleset.  that's a serious
> bug....

I don't mind using eval around things, but the empty ruleset thing is bug 4941.
;)    

Taking a quick look through the CHANGES files, it looks like we should require
Archive::Tar 1.23 and IO::Zlib 1.04.  I'll update the docs and see about getting
together a patch as well.
Comment 9 Theo Van Dinter 2006-07-19 15:38:07 UTC
Created attachment 3596 [details]
suggested patch

as committed to 3.2:
Sending        lib/Mail/SpamAssassin/Util/DependencyInfo.pm
Sending        sa-update.raw
Transmitting file data ..
Committed revision 423499.
Comment 10 Justin Mason 2006-07-20 21:53:56 UTC
+1 on that patch.

hmm.  actually, I can't see how the IO::Zlib failure could wind up with an empty
ruleset, so I guess it must be bug 4941 ;)
Comment 11 Theo Van Dinter 2006-07-20 22:56:50 UTC
(In reply to comment #10)
> hmm.  actually, I can't see how the IO::Zlib failure could wind up with an empty
> ruleset, so I guess it must be bug 4941 ;)

The problem is that on the first sa-update run, the new local state directory is
made at the start to hold the temp files, and then when the uncompress fails,
sa-update dies leaving an empty local state dir.  fwiw. :)
Comment 12 Justin Mason 2006-07-21 08:56:58 UTC
ahhh, that makes sense.  ok, agreed, that's definite a separate bug.
Comment 13 Doc Schneider 2006-07-21 12:49:42 UTC
+1 on the patch.
Comment 14 Theo Van Dinter 2006-07-21 14:48:18 UTC
Sending        lib/Mail/SpamAssassin/Util/DependencyInfo.pm
Sending        sa-update.raw
Transmitting file data ..
Committed revision 424340.


I'm going to resolve this for now.  JM (or anyone else), if you want to go ahead
and wrap the various external module calls with eval{}, go for it.
Comment 15 Justin Mason 2006-07-21 15:02:58 UTC
'if you want to go ahead
and wrap the various external module calls with eval{}, go for it.'

cheers.  I'll give that a miss.  I took a look, and a die() is actually the
appropriate action to happen at those spots -- it won't cause trouble once bug
4941 is fixed.