[Bug 7065] New: Debug Mode breaks Bayes but only if DBM storage is used

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7065

            Bug ID: 7065
           Summary: Debug Mode breaks Bayes but only if DBM storage is
                    used
           Product: Spamassassin
           Version: 3.4.0
          Hardware: PC
                OS: FreeBSD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Learner
          Assignee: dev@spamassassin.apache.org
          Reporter: h.skuhra@gmail.com

Running spamassassin 3.4.0 in debug mode results in

warn: plugin: eval failed: Insecure dependency in sprintf while running with -T
switch at /usr/local/lib/perl5/site_perl/5.16/Mail/SpamAssassin/Logger.pm line
241.

and disables Bayes Plugin. 
This happens only with Mail::SpamAssassin::BayesStore::DBM. No problem with BDB
and SDBM.

The patch from
<http://mail-archives.apache.org/mod_mbox/spamassassin-users/201407.mbox/%3Calpine.LNX.2.00.1407050856170.29575%40athena.impsec.org%3E>
fixes the issue.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7065] Debug Mode breaks Bayes but only if DBM storage is used

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7065

Kevin A. McGrail <km...@pccc.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Kevin A. McGrail <km...@pccc.com> ---
As we are in commit then review, it's trunk and Mark's is an implicit +1
anyway...

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7065] Debug Mode breaks Bayes but only if DBM storage is used

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7065

Kevin A. McGrail <km...@pccc.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kmcgrail@pccc.com

--- Comment #6 from Kevin A. McGrail <km...@pccc.com> ---
(In reply to Mark Martinec from comment #5)
> Bug 7065: Debug Mode breaks Bayes but only if DBM storage is used
>   Sending BayesStore/DBM.pm
> Committed revision 1608413.
> 
> Took the liberty to also change a couple of other dbg() calls
> in that module into a sprintf form. I know that JM preferred
> to have debug strings containing interpolated variables, instead
> of having them decoupled through formatting. It's hard to argue
> personal preferences regarding which is easier to read,
> but I also claim one technical advantage: passing additional
> arguments in a subroutine call is cheap as it only involves
> aliasing, not copying. On the other hand, constructing a
> complete debug string by a caller, only to be later discarded
> by a logging routine (unless debugging is enabled) sounds
> like a waste.

Sounds good to me and a good find on why it breaks that form of Bayes.  This
can be marked resolved/fixed, yes?

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7065] Debug Mode breaks Bayes but only if DBM storage is used

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7065

--- Comment #5 from Mark Martinec <Ma...@ijs.si> ---
Bug 7065: Debug Mode breaks Bayes but only if DBM storage is used
  Sending BayesStore/DBM.pm
Committed revision 1608413.

Took the liberty to also change a couple of other dbg() calls
in that module into a sprintf form. I know that JM preferred
to have debug strings containing interpolated variables, instead
of having them decoupled through formatting. It's hard to argue
personal preferences regarding which is easier to read,
but I also claim one technical advantage: passing additional
arguments in a subroutine call is cheap as it only involves
aliasing, not copying. On the other hand, constructing a
complete debug string by a caller, only to be later discarded
by a logging routine (unless debugging is enabled) sounds
like a waste.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7065] Debug Mode breaks Bayes but only if DBM storage is used

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7065

--- Comment #1 from Mark Martinec <Ma...@ijs.si> ---
> -  dbg("bayes: DB journal sync: last sync: ".$vars[7],'bayes','-1');
> +  dbg("bayes: DB journal sync: last sync: ".$vars[7]);

That's still not the right way to do it. The (tainted) $vars[7] is treated
as part of a sprintf format string, which is unsafe and is the reason
for the original failure. Instead, it should be an argument to a %s:

-  dbg("bayes: DB journal sync: last sync: ".$vars[7],'bayes','-1');
+  dbg("bayes: DB journal sync: last sync: %s", $vars[7]);

Not sure what was the purpose of 'bayes','-1' arguments.
Looks like some leftover.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7065] Debug Mode breaks Bayes but only if DBM storage is used

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7065

--- Comment #4 from Mark Martinec <Ma...@ijs.si> ---
> Not sure what was the purpose of 'bayes','-1' arguments.
> Looks like some leftover.

Digging into old versions I can see that in 3.0.1 the
dbg() would still accept two additional arguments:

sub dbg {
  ...
  my ($msg, $codepath, $level) = @_;

so these is what 'bayes','-1' represented.

The 3.1.0 changed this and level became implied by calling
either dbg() or info(). Changed some time in 2005.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7065] Debug Mode breaks Bayes but only if DBM storage is used

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7065

--- Comment #2 from Mark Martinec <Ma...@ijs.si> ---
> Not sure what was the purpose of 'bayes','-1' arguments.

Btw, the reason why just removing these extra arguments avoids
the taint problem is that for compatibility reasons the logger
treats the first argument to dbg() as a plain string (not a
sprintf format) when there are no additional arguments specified
in a call to dbg() or info().

So yes, the John Hardin's patch also does solve the problem
just by removing the extra parameters in a dbg() call.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7065] Debug Mode breaks Bayes but only if DBM storage is used

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7065

Mark Martinec <Ma...@ijs.si> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|Undefined                   |3.4.1
                 OS|FreeBSD                     |All

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7065] Debug Mode breaks Bayes but only if DBM storage is used

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7065

--- Comment #7 from John Hardin <jh...@impsec.org> ---
(In reply to Kevin A. McGrail from comment #6)
> (In reply to Mark Martinec from comment #5)
> > constructing a
> > complete debug string by a caller, only to be later discarded
> > by a logging routine (unless debugging is enabled) sounds
> > like a waste.

Agreed.

> This can be marked resolved/fixed, yes?

+1 from me.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7065] Debug Mode breaks Bayes but only if DBM storage is used

[bu...@bugzilla.spamassassin.org]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=7065

John Hardin <jh...@impsec.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jhardin@impsec.org

--- Comment #3 from John Hardin <jh...@impsec.org> ---
(In reply to Mark Martinec from comment #1)
> > -  dbg("bayes: DB journal sync: last sync: ".$vars[7],'bayes','-1');
> > +  dbg("bayes: DB journal sync: last sync: ".$vars[7]);
> 
> That's still not the right way to do it.

I didn't suggest that it was a complete or correct solution to the underlying
problem, only that it might make SA stop blowing up in a way that killed
Bayes... :)

> The (tainted) $vars[7] is treated
> as part of a sprintf format string, which is unsafe and is the reason
> for the original failure.

Right.

> Instead, it should be an argument to a %s:
> 
> -  dbg("bayes: DB journal sync: last sync: ".$vars[7],'bayes','-1');
> +  dbg("bayes: DB journal sync: last sync: %s", $vars[7]);

As $vars[7] is a number I was also thinking of something like $vars[7] =~ /\d+/
just before that dbg() call to untaint it, but your proposal is probably better
and simpler overall.

+1.

> Not sure what was the purpose of 'bayes','-1' arguments.
> Looks like some leftover.

Agreed, that's why I suggested removing them as a q'n'd fix.

-- 
You are receiving this mail because:
You are the assignee for the bug.