SA Bugzilla – Bug 4997
[review] sa-update triggers errors in IO/Zlib.pm
Last modified: 2006-07-21 08:02:58 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
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 :(
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.
(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.
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.
(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.
> 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.
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.
(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.
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.
+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 ;)
(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. :)
ahhh, that makes sense. ok, agreed, that's definite a separate bug.
+1 on the patch.
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.
'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.