Bug 5786 - [review] SpamAssassin 3.2.4 introduces nasty new warnings with Mail::DKIM 0.30
Summary: [review] SpamAssassin 3.2.4 introduces nasty new warnings with Mail::DKIM 0.30
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Plugins (show other bugs)
Version: 3.2.4
Hardware: PC Linux
: P5 normal
Target Milestone: 3.2.5
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard: ready to commit
Keywords:
Depends on:
Blocks:
 
Reported: 2008-01-14 16:22 UTC by Nick Alcock
Modified: 2009-09-27 17:40 UTC (History)
1 user (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Patch to fix breakage with Mail::DKIM 0.30 patch None Nick Alcock [HasCLA]
fix for a missing From with DKIM, and a fix to a SPF test mail patch None Mark Martinec [HasCLA]
Show Obsolete (1) Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Alcock 2008-01-14 16:22:58 UTC 
These are SA 3.2.4 test results:

t/spf.............................97/106 [27086] warn: Use of uninitialized 
value in substitution (s///) at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm 
line 338.
[27086] warn: Use of uninitialized value in substitution (s///) 
at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm line 338.
[27086] warn: Use of uninitialized value in string ne 
at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm line 339.
t/spf.............................99/106 [27090] warn: Use of uninitialized 
value in substitution (s///) at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm 
line 338.
[27090] warn: Use of uninitialized value in substitution (s///) 
at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm line 338.
[27090] warn: Use of uninitialized value in string ne 
at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm line 339.
t/spf.............................101/106 [27094] warn: Use of uninitialized 
value in substitution (s///) at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm 
line 338.
[27094] warn: Use of uninitialized value in substitution (s///) 
at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm line 338.
[27094] warn: Use of uninitialized value in string ne 
at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm line 339.
t/spf.............................103/106 [27098] warn: Use of uninitialized 
value in substitution (s///) at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm 
line 338.
[27098] warn: Use of uninitialized value in substitution (s///) 
at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm line 338.
[27098] warn: Use of uninitialized value in string ne 
at ../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm line 339.
t/spf.............................ok
Ironically, this bug was introduced by Mark Martinec's Mail::DKIM 0.30-support 
patch, which failed to note the effects of this change to Mail::DKIM:

2007-11-14: Jason Long <jlong@messiah.edu>
 * lib/Mail/DKIM/Verifier.pm, Signer.pm: update documentation for
   message_sender() and message_originator() methods, which are now
   guaranteed to return an object
 * lib/Mail/DKIM/Common.pm (message_sender, message_originator):
   always return a Mail::Address object, even if the relevant headers
   were not found

Patch attached.
Comment 1 Nick Alcock 2008-01-14 16:25:02 UTC 
Created attachment 4236 [details]
Patch to fix breakage with Mail::DKIM 0.30

A trivial patch to cater for the verifier's message_originator method in
Mail::DKIM 0.30 always returning an address which may be empty if there is no
originator, rather than returning undef in that case.
Comment 2 Mark Martinec 2008-01-15 02:33:26 UTC 
> A trivial patch to cater for the verifier's message_originator method in
> Mail::DKIM 0.30 always returning an address which may be empty if there is no
> originator, rather than returning undef in that case.

Thanks, applied to trunk:

svn -m 'DKIM: avoid a "Use of uninitialized value" warning
  when a From header field is missing in a message, Bug 5786' ci
Sending        lib/Mail/SpamAssassin/Plugin/DKIM.pm
Committed revision 612071.

Messages without a From header field are rarely seen, so the warning
does not occur often in practice.

This leaves a question why the SPF test file t/data/nice/spf3-received-spf
contains two empty lines in the middle of a header, pushing the last
couple of header fields (along with a From) into a body. Same in 3.2.4
and in trunk.
Comment 3 Nick Alcock 2008-01-15 12:29:59 UTC 
Isn't it obvious? To find bugs like this one!

Seriously, buggy MTAs, buggy milters, and buggy procmail recipes can all dump 
extra linefeeds into headers, and while SA can hardly operate perfectly with 
the headers truncated like that, it shouldn't give out uninitialized variable 
errors.

(Agreed that this is an unlikely bug, though.)
Comment 4 Karsten Bräckelmann 2008-01-26 21:43:47 UTC 
Any plans for 3.2.5?  Or why is this bug still open? ;)
Comment 5 Justin Mason 2008-01-27 03:46:02 UTC 
I presume the plans are to apply this to 3.2.5 as well
Comment 6 Mark Martinec 2008-01-28 10:42:14 UTC 
> I presume the plans are to apply this to 3.2.5 as well

Sure, can't hurt. Will there be a 3.2.5?

> This leaves a question why the SPF test file t/data/nice/spf3-received-spf
> contains two empty lines in the middle of a header, pushing the last
> couple of header fields (along with a From) into a body. Same in 3.2.4
> and in trunk.

Can I assume this is a bug and the test message should be fixed too?
If so, I'll prepare a (trivial) combined patch to get both changes to 3.2.
Comment 7 Daryl C. W. O'Shea 2008-01-28 11:16:06 UTC 
> > This leaves a question why the SPF test file t/data/nice/spf3-received-spf
> > contains two empty lines in the middle of a header, pushing the last
> > couple of header fields (along with a From) into a body. Same in 3.2.4
> > and in trunk.
> 
> Can I assume this is a bug and the test message should be fixed too?
> If so, I'll prepare a (trivial) combined patch to get both changes to 3.2.

Yes.  I have no idea how those empty lines showed up in r487146.
Comment 8 Justin Mason 2008-01-28 12:14:03 UTC 
(In reply to comment #6)
> > I presume the plans are to apply this to 3.2.5 as well
> 
> Sure, can't hurt. Will there be a 3.2.5?

IMO, yes, we should do one -- bug 5769 at least is ugly enough.

> > This leaves a question why the SPF test file t/data/nice/spf3-received-spf
> > contains two empty lines in the middle of a header, pushing the last
> > couple of header fields (along with a From) into a body. Same in 3.2.4
> > and in trunk.
> 
> Can I assume this is a bug and the test message should be fixed too?
> If so, I'll prepare a (trivial) combined patch to get both changes to 3.2.

+1
Comment 9 Mark Martinec 2008-02-06 16:29:19 UTC 
Created attachment 4251 [details]
fix for a missing From with DKIM, and a fix to a SPF test mail

combined fix, trivial
Comment 10 Justin Mason 2008-02-07 01:33:13 UTC 
+1
Comment 11 Sidney Markowitz 2008-02-07 04:27:43 UTC 
+1
Comment 12 Mark Martinec 2008-02-07 05:23:51 UTC 
$ svn -m 'bug 5786: fix for missing From with DKIM;
  remove empty lines from SPF test mail header' ci
Sending        lib/Mail/SpamAssassin/Plugin/DKIM.pm
Sending        t/data/nice/spf3-received-spf
Committed revision 619403.