[Bug 6335] [review] add domain-only and IP-only lookups for URIDNSBL, for Spamhaus DBL

[Karsten Bräckelmann <gu...@rudersport.de>]

(Sorry for the broken threading. Started typing, before picking one of
the many possible posts for reply.)

I think I might have found a corner-case problem with the patch (in
trunk) for bug 6335. The problem appears to be, that $dnsbl_lookup_ips
and $is_ip are not independent.

+  my $cf = $scanner->{uridnsbl_active_rules_revipbl};
+  my $dnsbl_lookup_ips = 0;
+  foreach my $rulename (keys %{$cf}) {
+    if ($tflags->{$rulename} !~ /\bdomains_only\b/) {
+      $dnsbl_lookup_ips++;

$dnsbl_lookup_ips == 0  IFF *all* $tflags->{$rulename} *do*
match /domains_only/ (assumption (1)).

Due to

+  if ($dnsbl_lookup_ips && $dom =~ /^\d+\.\d+\.\d+\.\d+$/) {

$is_ip then also is 0, even if $dom indeed *is* an IP.

+      next if ($is_ip && $tflags->{$rulename} =~ /\bdomains_only\b/);

This test later on then fails to skip, because $is_ip is zero. Again,
even in the case of $dom actually being an IP. And the respective tflags
set to domains_only, as per the corner-case assumption of *all* such
rules having that tflag set.


Caveat: I did not yet figure out, which rules actually are in
$scanner->{uridnsbl_active_rules_revipbl};  which is the sub-set of
rules that need to satisfy the assumption (a).


-- 
char *t="\10pse\0r\0dtu\0.@ghno\x4e\xc8\x79\xf4\xab\x51\x8a\x10\xf4\xf4\xc4";
main(){ char h,m=h=*t++,*x=t+2*h,c,i,l=*x,s=0; for (i=0;i<l;i++){ i%8? c<<=1:
(c=*++x); c&128 && (s+=h); if (!(h>>=1)||!t[s+h]){ putchar(t[s]);h=m;s=0; }}}

Re: [Bug 6335] [review] add domain-only and IP-only lookups for URIDNSBL, for Spamhaus DBL

[Karsten Bräckelmann <gu...@rudersport.de>]

Trying to keep the noise out of bugzilla, and dump facts there instead
after discussion. :)

On Wed, 2010-03-03 at 12:09 +0000, Justin Mason wrote:
> yeah, you're right  :(  I'll have to work on that.

Shouldn't $dnsbl_lookup_ips and $is_ip just be independent?

Is $dnsbl_lookup_ips even necessary? All it tracks is, whether there is
a single rule with tflags *not* set to domains_only. Even if we can fix
that logic, it seems hardly necessary to guard the IP quad reversion by
that -- it's a rare corner case, and comes at the cost of a foreach loop
and more complex code. I'd go with unconditional reversion and just skip
the rev IP later for the test that's set to tflags domains_only.


> 2010/3/2 Karsten Bräckelmann <gu...@rudersport.de>:
> > I think I might have found a corner-case problem with the patch (in
> > trunk) for bug 6335. The problem appears to be, that $dnsbl_lookup_ips
> > and $is_ip are not independent.
[...]

> > +      next if ($is_ip && $tflags->{$rulename} =~ /\bdomains_only\b/);

This one and the following "inversed next" appear to be the core logic.
Any what is actually required, given $is_ip is kept to distinguish
between IPs and domains.


> > Caveat: I did not yet figure out, which rules actually are in
> > $scanner->{uridnsbl_active_rules_revipbl};  which is the sub-set of
> > rules that need to satisfy the assumption (a).

Would really help code review, if I knew what that actually is, and
where it previously has been defined.

What I wonder in this context is, why it has been introduced into
query_domain() for the patch.


-- 
char *t="\10pse\0r\0dtu\0.@ghno\x4e\xc8\x79\xf4\xab\x51\x8a\x10\xf4\xf4\xc4";
main(){ char h,m=h=*t++,*x=t+2*h,c,i,l=*x,s=0; for (i=0;i<l;i++){ i%8? c<<=1:
(c=*++x); c&128 && (s+=h); if (!(h>>=1)||!t[s+h]){ putchar(t[s]);h=m;s=0; }}}

Re: [Bug 6335] [review] add domain-only and IP-only lookups for URIDNSBL, for Spamhaus DBL

[Justin Mason <jm...@jmason.org>]

yeah, you're right  :(  I'll have to work on that.

2010/3/2 Karsten Bräckelmann <gu...@rudersport.de>:
> (Sorry for the broken threading. Started typing, before picking one of
> the many possible posts for reply.)
>
> I think I might have found a corner-case problem with the patch (in
> trunk) for bug 6335. The problem appears to be, that $dnsbl_lookup_ips
> and $is_ip are not independent.
>
> +  my $cf = $scanner->{uridnsbl_active_rules_revipbl};
> +  my $dnsbl_lookup_ips = 0;
> +  foreach my $rulename (keys %{$cf}) {
> +    if ($tflags->{$rulename} !~ /\bdomains_only\b/) {
> +      $dnsbl_lookup_ips++;
>
> $dnsbl_lookup_ips == 0  IFF *all* $tflags->{$rulename} *do*
> match /domains_only/ (assumption (1)).
>
> Due to
>
> +  if ($dnsbl_lookup_ips && $dom =~ /^\d+\.\d+\.\d+\.\d+$/) {
>
> $is_ip then also is 0, even if $dom indeed *is* an IP.
>
> +      next if ($is_ip && $tflags->{$rulename} =~ /\bdomains_only\b/);
>
> This test later on then fails to skip, because $is_ip is zero. Again,
> even in the case of $dom actually being an IP. And the respective tflags
> set to domains_only, as per the corner-case assumption of *all* such
> rules having that tflag set.
>
>
> Caveat: I did not yet figure out, which rules actually are in
> $scanner->{uridnsbl_active_rules_revipbl};  which is the sub-set of
> rules that need to satisfy the assumption (a).
>
>
> --
> char *t="\10pse\0r\0dtu\0.@ghno\x4e\xc8\x79\xf4\xab\x51\x8a\x10\xf4\xf4\xc4";
> main(){ char h,m=h=*t++,*x=t+2*h,c,i,l=*x,s=0; for (i=0;i<l;i++){ i%8? c<<=1:
> (c=*++x); c&128 && (s+=h); if (!(h>>=1)||!t[s+h]){ putchar(t[s]);h=m;s=0; }}}
>
>



-- 
--j.