You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spamassassin.apache.org by bu...@bugzilla.spamassassin.org on 2015/03/04 19:09:54 UTC
[Bug 7153] New: URILocalBL.pm may still leak messages to stderr
under certain circumstances
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7153
Bug ID: 7153
Summary: URILocalBL.pm may still leak messages to stderr under
certain circumstances
Product: Spamassassin
Version: SVN Trunk (Latest Devel Version)
Hardware: All
OS: All
Status: NEW
Severity: normal
Priority: P2
Component: Plugins
Assignee: dev@spamassassin.apache.org
Reporter: philipp@redfish-solutions.com
Created attachment 5281
--> https://bz.apache.org/SpamAssassin/attachment.cgi?id=5281&action=edit
Include test for correct library version
If you're using a fairly recent (1.4.4 or later) version of geoip-api-perl (aka
perl-Geo-IP, etc) but an old version of the actual C libraries, you can still
get into a situation where you leak messages to stderr, causing all sorts of
trouble.
The workaround is to ask the C library itself (via an XS stub in Perl) what its
version is, and only if it's 1.6.3 or later attempt to use the GEOIP_SILENCE
flag.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7153] URILocalBL.pm may still leak messages to stderr under
certain circumstances
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7153
--- Comment #2 from Philip Prindeville <ph...@redfish-solutions.com> ---
(In reply to Kevin A. McGrail from comment #1)
> svn commit -m 'Bug 7153 for leak of URILocalBL.pm to stderr'
> Sending lib/Mail/SpamAssassin/Plugin/URILocalBL.pm
> Transmitting file data .
> Committed revision 1665706
>
> This is a follow-up on bug 7079.
>
> You are defining $gic_wanted and have but as best I can tell, never using
> those variables.
>
> Did you mean to have && $gic_wanted >= $gic_have as well in the conditions?
It turns out that if you don't need to test the C and Perl version, just the...
oh, bugger. Just the C version. That's not what I did, is it?
If the Perl code is defining Geo::IP::GEOIP_SILENCE then it's a valid version.
But we still need to test for the underlying C code to support that flag.
> I added that for now. Otherwise I would comment out the $gic_wanted/have
> definition.
>
> Considering resolved unless you say otherwise Philip.
The correct test should have been:
if ($flags && $gic_wanted >= $gic_have) {
and I'm an idiot. Can you please fix that?
We could have done the earlier sequence:
my $flags = 0;
eval '$flags = Geo::IP::GEOIP_SILENCE' if ($gip_wanted >= $gip_have);
instead, but it being defined is implied by having the correct version and vice
versa.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7153] URILocalBL.pm may still leak messages to stderr under
certain circumstances
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7153
Kevin A. McGrail <km...@pccc.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |FIXED
Status|REOPENED |RESOLVED
--- Comment #5 from Kevin A. McGrail <km...@pccc.com> ---
Hopefully fixed now.
svn commit -m 'more cleanup for bug 7153'
Sending lib/Mail/SpamAssassin/Plugin/URILocalBL.pm
Transmitting file data .
Committed revision 1665864.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7153] URILocalBL.pm may still leak messages to stderr under
certain circumstances
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7153
--- Comment #6 from Philip Prindeville <ph...@redfish-solutions.com> ---
(In reply to Kevin A. McGrail from comment #5)
> Hopefully fixed now.
Yup. Looks good. Running it locally.
The plugin should have addressed this issue sooner but I needed to get it fixed
upstream by the vendor (MaxMind) and then the distro I test on (Fedora) to push
out updated packages with the correct upstream version, etc.
Lots of process.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7153] URILocalBL.pm may still leak messages to stderr under
certain circumstances
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7153
Kevin A. McGrail <km...@pccc.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |FIXED
Status|NEW |RESOLVED
--- Comment #1 from Kevin A. McGrail <km...@pccc.com> ---
svn commit -m 'Bug 7153 for leak of URILocalBL.pm to stderr'
Sending lib/Mail/SpamAssassin/Plugin/URILocalBL.pm
Transmitting file data .
Committed revision 1665706
This is a follow-up on bug 7079.
You are defining $gic_wanted and have but as best I can tell, never using those
variables.
Did you mean to have && $gic_wanted >= $gic_have as well in the conditions?
I added that for now. Otherwise I would comment out the $gic_wanted/have
definition.
Considering resolved unless you say otherwise Philip.
Index: lib/Mail/SpamAssassin/Plugin/URILocalBL.pm
===================================================================
--- lib/Mail/SpamAssassin/Plugin/URILocalBL.pm (revision 1665676)
+++ lib/Mail/SpamAssassin/Plugin/URILocalBL.pm (working copy)
@@ -110,7 +110,14 @@
use warnings;
use bytes;
use re 'taint';
+use version;
+# need GeoIP C library 1.6.3 and GeoIP perl API 1.4.4 or later to avoid
messages leaking - Bug 7153
+my $gic_wanted = version->parse('v1.4.4');
+my $gic_have = version->parse(Geo::IP->lib_version());
+my $gip_wanted = version->parse('v1.6.3');
+my $gip_have = version->parse($Geo::IP::VERSION);
+
use vars qw(@ISA);
@ISA = qw(Mail::SpamAssassin::Plugin);
@@ -132,10 +139,8 @@
# this code burps an ugly message if it fails, but that's redirected
elsewhere
my $flags = 0;
eval '$flags = Geo::IP::GEOIP_SILENCE if (defined &Geo::IP::GEOIP_SILENCE)';
- open(STDERR, ">&OLDERR");
- close(OLDERR);
- if ($flags) {
+ if ($flags && $gip_wanted >= $gip_have && $gic_wanted >= $gic_have) {
$self->{geoip} = Geo::IP->new(GEOIP_MEMORY_CACHE | GEOIP_CHECK_CACHE |
$flags);
$self->{geoisp} = Geo::IP->open_type(GEOIP_ISP_EDITION, GEOIP_MEMORY_CACHE
| GEOIP_CHECK_CACHE | $flags);
} else {
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7153] URILocalBL.pm may still leak messages to stderr under
certain circumstances
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7153
Kevin A. McGrail <km...@pccc.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |REOPENED
Resolution|FIXED |---
--- Comment #3 from Kevin A. McGrail <km...@pccc.com> ---
Is this closer to what we need? Also trying to figure out whether gic needs to
be 1.4.4 or 1.6.3 as the comment states differently than the patch.
Index: lib/Mail/SpamAssassin/Plugin/URILocalBL.pm
===================================================================
--- lib/Mail/SpamAssassin/Plugin/URILocalBL.pm (revision 1665752)
+++ lib/Mail/SpamAssassin/Plugin/URILocalBL.pm (working copy)
@@ -113,9 +113,9 @@
use version;
# need GeoIP C library 1.6.3 and GeoIP perl API 1.4.4 or later to avoid
messages leaking - Bug 7153
-my $gic_wanted = version->parse('v1.4.4');
+my $gic_wanted = version->parse('v1.6.3');
my $gic_have = version->parse(Geo::IP->lib_version());
-my $gip_wanted = version->parse('v1.6.3');
+my $gip_wanted = version->parse('v1.4.4');
my $gip_have = version->parse($Geo::IP::VERSION);
use vars qw(@ISA);
@@ -138,9 +138,9 @@
# this code burps an ugly message if it fails, but that's redirected
elsewhere
my $flags = 0;
- eval '$flags = Geo::IP::GEOIP_SILENCE if (defined &Geo::IP::GEOIP_SILENCE)';
+ eval '$flags = Geo::IP::GEOIP_SILENCE' if ($gip_wanted >= $gip_have);
- if ($flags && $gip_wanted >= $gip_have && $gic_wanted >= $gic_have) {
+ if ($flags && $gic_wanted >= $gic_have) {
$self->{geoip} = Geo::IP->new(GEOIP_MEMORY_CACHE | GEOIP_CHECK_CACHE |
$flags);
$self->{geoisp} = Geo::IP->open_type(GEOIP_ISP_EDITION, GEOIP_MEMORY_CACHE
| GEOIP_CHECK_CACHE | $flags);
} else {
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7153] URILocalBL.pm may still leak messages to stderr under
certain circumstances
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7153
--- Comment #7 from Kevin A. McGrail <km...@pccc.com> ---
Understood!
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7153] URILocalBL.pm may still leak messages to stderr under
certain circumstances
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7153
Philip Prindeville <ph...@redfish-solutions.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |philipp@redfish-solutions.c
| |om
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7153] URILocalBL.pm may still leak messages to stderr under
certain circumstances
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7153
Philip Prindeville <ph...@redfish-solutions.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |kmcgrail@pccc.com
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7153] URILocalBL.pm may still leak messages to stderr under
certain circumstances
Posted by bu...@bugzilla.spamassassin.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7153
--- Comment #4 from Philip Prindeville <ph...@redfish-solutions.com> ---
(In reply to Kevin A. McGrail from comment #3)
> Is this closer to what we need? Also trying to figure out whether gic needs
> to be 1.4.4 or 1.6.3 as the comment states differently than the patch.
Yeah, I had the $gic_have and $gip_have (GeoIP-C and GeoIP-Perl, respectively)
backasswards.
Good catch.
--
You are receiving this mail because:
You are the assignee for the bug.