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 {

(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.

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 {

(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.

Hopefully fixed now.

svn commit -m 'more cleanup for bug 7153'
Sending        lib/Mail/SpamAssassin/Plugin/URILocalBL.pm
Transmitting file data .
Committed revision 1665864.

(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.

Understood!