You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spamassassin.apache.org by Alex Regan <my...@gmail.com> on 2015/01/14 18:06:23 UTC
Uninitialized value in PerMsgStatus
Hi,
I'm using amavisd-new-2.9.1 and perl-5.18.4 on fedora20 with the svn
spamassassin snapshot from today, and receive the following message:
Jan 14 11:59:21 mail01 amavis[19431]: (19431-18) _WARN: Use of
uninitialized value in concatenation (.) or string at
/usr/share/perl5/vendor_perl/Mail/SpamAssassin/PerMsgStatus.pm line
3072, <GEN19> line 16875.
3071 #loop through and limit to just the domain with a dummy address
3072 for (my $i = 0; $i < scalar(@addrs); $i++) {
3073 $addrs[$i] =
'dummy@'.&Mail::SpamAssassin::Util::uri_to_domain($addrs[$i]);
3074 }
This has apparently been going on for some time, not just with today's
update. I don't see any references to others having this problem, so
hoped someone could help.
How can I troubleshoot this? Could it be an amavis issue?
Thanks,
Alex
Re: Uninitialized value in PerMsgStatus
Posted by "Kevin A. McGrail" <KM...@PCCC.com>.
On 1/15/2015 2:43 PM, Mark Martinec wrote:
>> What is a PR? A bugzilla ticket?
>
> Yes, a problem report. Sorry for using terminology from another context.
Thanks. I wasn't sure if you meant a press release about the bug but
that seemed overkill...
>> If there are other perl critic warnings, they are likely not enabled
>> in that test.
>
> Opened a Bug 7119.
> Probably depends on a version of Perl::Critic.
I upgraded to $VERSION = '1.121'; of Perl::Critic so we can try and be
on the same page.
> That statement deserves the '## no critic' annotation,
> something like (untested) :
>
> # see Bug 7120
> return undef if !defined $_[0]; ## no critic
> (ProhibitExplicitReturnUndef)
Exactly. ## no critic (ProhibitExplicitReturnUndef) - See Bug 7120
added and passes xt test.
Will commit as soon as I have a full make test completed.
>
> For other cases I didn't check.
The other cases are all returns if a $self checker is not defined so
much less of a concern anyway.
Regards,
KAM
Re: Uninitialized value in PerMsgStatus
Posted by Mark Martinec <Ma...@ijs.si>.
>> There are some other perlcritic warnings about modifying $_ in list
>> functions
>> (in sa-update, spamassassin, spamd). Opening a PR would be warranted.
> What is a PR? A bugzilla ticket?
Yes, a problem report. Sorry for using terminology from another
context.
> If there are other perl critic warnings, they are likely not enabled
> in that test.
Opened a Bug 7119.
Probably depends on a version of Perl::Critic.
> # Perl::Critic found these violations in
> "../blib/lib/Mail/SpamAssassin/Util.pm":
> # "return" statement with explicit "undef" at line 287, column 5.
Opened a Bug 7120. It can be used as a reference in a comment
when adding "## no critic" annotations in code.
> Looking back, this return undef was added after 3.4.0's release and
> appears to have been added during the profiling work on
> https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6854
I probably fixed that potential or actual problem in passing.
>> The MS::Util::untaint_var() is still broken...
> I'm reverting some recent changes and reverting back to the one undef
Thanks!
> Do you feel confident enough about your change that I should modify or
> notate the build process notes to ignore that issue for that specific
> return undef?
For the 'return undef' in untaint_var() most definitely so.
That function can be used when assembling arguments in a
call to some other function, just as described in Bug 7120.
That statement deserves the '## no critic' annotation,
something like (untested) :
# see Bug 7120
return undef if !defined $_[0]; ## no critic
(ProhibitExplicitReturnUndef)
For other cases I didn't check.
>> Btw, common use of 'return 0' in TxRep to indicate a boolean response
>> falls into the same category as 'return undef', although
>> Test::Perl::Critic
>> does not check for that.
> I use no warnings 'uninitialized', 'once'; for a lot of my day to day
> work
> so I can see that but my key concern is passing the tests required of a
> release without trying to change those tests right now.
Testing in a boolean context does not issue a warning when
an expression is undefined. There is no reason to return
a zero (instead of undef or an empty list) when a subroutine
result is considered a boolean value.
Mark
Re: Uninitialized value in PerMsgStatus
Posted by Quanah Gibson-Mount <qu...@zimbra.com>.
--On Thursday, January 15, 2015 11:48 AM -0500 "Kevin A. McGrail"
<KM...@PCCC.com> wrote:
>> (in sa-update, spamassassin, spamd). Opening a PR would be warranted.
> What is a PR? A bugzilla ticket?
PR is generally short for "problem report". So yes, I'd guess a bugzilla
ticket.
--Quanah
--
Quanah Gibson-Mount
Platform Architect
Zimbra, Inc.
--------------------
Zimbra :: the leader in open source messaging and collaboration
Re: Uninitialized value in PerMsgStatus
Posted by "Kevin A. McGrail" <KM...@PCCC.com>.
On 1/15/2015 11:09 AM, Mark Martinec wrote:
>> :-) Well I don't want to change the vetting process for a release and
>> xt/60_perlcritic.t has been used for years on the code base.
>> Suggestions what we can do to resolve the issue that also passes that
>> test so we don't have to go down that rabbithole?
>
> There are some other perlcritic warnings about modifying $_ in list
> functions
> (in sa-update, spamassassin, spamd). Opening a PR would be warranted.
What is a PR? A bugzilla ticket?
Also, running prove -v xt/60_perlcritic.t shows just the one issue and I
committed the change which reverses some fixes and the original return
undef change in Util.pm.
not ok 38 - Test::Perl::Critic for "../blib/lib/Mail/SpamAssassin/Util.pm"
# Failed test 'Test::Perl::Critic for
"../blib/lib/Mail/SpamAssassin/Util.pm"'
# at /usr/local/lib/perl5/site_perl/5.8.6/Test/Perl/Critic.pm line 110.
#
# Perl::Critic found these violations in
"../blib/lib/Mail/SpamAssassin/Util.pm":
# "return" statement with explicit "undef" at line 287, column 5.
If there are other perl critic warnings, they are likely not enabled in
that test.
Looking back, this return undef was added after 3.4.0's release and
appears to have been added during the profiling work on
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6854
Do you feel confident enough about your change that I should modify or
notate the build process notes to ignore that issue for that specific
return undef?
> The MS::Util::untaint_var() is still broken...
I'm reverting some recent changes and reverting back to the one undef in
there. Sorry about that. Best of intentions.
> Btw, common use of 'return 0' in TxRep to indicate a boolean response
> falls into the same category as 'return undef', although
> Test::Perl::Critic
> does not check for that.
I use no warnings 'uninitialized', 'once'; for a lot of my day to day
work so I can see that but my key concern is passing the tests required
of a release without trying to change those tests right now.
For a decade or so, we've been able to make releases that pass that
60_perlcritic test and don't cause real-world issues. I'm confident
that we can do so at least in this case. I want to get the release out
by Jan 30. Then I think you are correct we need to look at if this
makes sense longer-term.
Regards,
KAM
Re: Uninitialized value in PerMsgStatus
Posted by Mark Martinec <Ma...@ijs.si>.
> :-) Well I don't want to change the vetting process for a release and
> xt/60_perlcritic.t has been used for years on the code base.
> Suggestions what we can do to resolve the issue that also passes that
> test so we don't have to go down that rabbithole?
There are some other perlcritic warnings about modifying $_ in list
functions
(in sa-update, spamassassin, spamd). Opening a PR would be warranted.
The MS::Util::untaint_var() is still broken...
Btw, common use of 'return 0' in TxRep to indicate a boolean response
falls into the same category as 'return undef', although
Test::Perl::Critic
does not check for that.
Mark
Re: Uninitialized value in PerMsgStatus
Posted by "Kevin A. McGrail" <KM...@PCCC.com>.
On 1/14/2015 3:11 PM, Mark Martinec wrote:
>>> Yep that was done to pass the XT tests for a release. Joe can you
>>> look at those returns again?
>>
>> That advice from perlcritic needs to be taken with a large grain of
>> salt.
>> In the past I have been bitten by this several times. It is generally
>> safer to leave 'return undef' unless one carefully analyzes in what
>> context a subroutine is being called. I'd suggest to revert that
>> changeset entirely.
>> Mark
>
>
> If a subroutine is supposed to return exactly one scalar
> and is using a return, that return must be a 'return undef'.
>
> Consider the surprising behaviour:
>
> perl -le '
> sub foo {$a=shift; return if !$a; sin($a)};
> ($x, $y, $z) = (foo(3), foo(0), 9);
> print "x=$x, y=$y, z=$z"'
>
> x=0.141120008059867, y=9, z=
:-) Well I don't want to change the vetting process for a release and
xt/60_perlcritic.t has been used for years on the code base. Suggestions
what we can do to resolve the issue that also passes that test so we
don't have to go down that rabbithole?
Regards,
KAM
Re: Uninitialized value in PerMsgStatus
Posted by Mark Martinec <Ma...@ijs.si>.
>> Yep that was done to pass the XT tests for a release. Joe can you
>> look at those returns again?
>
> That advice from perlcritic needs to be taken with a large grain of
> salt.
> In the past I have been bitten by this several times. It is generally
> safer to leave 'return undef' unless one carefully analyzes in what
> context a subroutine is being called. I'd suggest to revert that
> changeset entirely.
> Mark
If a subroutine is supposed to return exactly one scalar
and is using a return, that return must be a 'return undef'.
Consider the surprising behaviour:
perl -le '
sub foo {$a=shift; return if !$a; sin($a)};
($x, $y, $z) = (foo(3), foo(0), 9);
print "x=$x, y=$y, z=$z"'
x=0.141120008059867, y=9, z=
Mark
Re: Uninitialized value in PerMsgStatus
Posted by Mark Martinec <Ma...@ijs.si>.
> Yep that was done to pass the XT tests for a release. Joe can you
> look at those returns again?
That advice from perlcritic needs to be taken with a large grain of
salt.
In the past I have been bitten by this several times. It is generally
safer to leave 'return undef' unless one carefully analyzes in what
context a subroutine is being called. I'd suggest to revert that
changeset entirely.
Mark
Re: Uninitialized value in PerMsgStatus
Posted by "Kevin A. McGrail" <KM...@PCCC.com>.
Yep that was done to pass the XT tests for a release. Joe can you look at those returns again?
Regards,
KAM
On January 14, 2015 1:35:20 PM EST, Mark Martinec <Ma...@ijs.si> wrote:
>2015-01-14 18:06, je Alex Regan napisal
>> Hi,
>>
>> I'm using amavisd-new-2.9.1 and perl-5.18.4 on fedora20 with the svn
>> spamassassin snapshot from today, and receive the following message:
>>
>> Jan 14 11:59:21 mail01 amavis[19431]: (19431-18) _WARN: Use of
>> uninitialized value in concatenation (.) or string at
>> /usr/share/perl5/vendor_perl/Mail/SpamAssassin/PerMsgStatus.pm line
>> 3072, <GEN19> line 16875.
>>
>> 3071 #loop through and limit to just the domain with a dummy
>address
>> 3072 for (my $i = 0; $i < scalar(@addrs); $i++) {
>> 3073 $addrs[$i] =
>> 'dummy@'.&Mail::SpamAssassin::Util::uri_to_domain($addrs[$i]);
>> 3074 }
>>
>> This has apparently been going on for some time, not just with
>today's
>> update. I don't see any references to others having this problem, so
>> hoped someone could help.
>>
>> How can I troubleshoot this? Could it be an amavis issue?
>>
>> Thanks,
>> Alex
>
>Looks like someone just recently blindly changed 'return undef'
>to a 'return', which breaks calls to such subroutine in a list
>context, e.g. foo( untaint($a), "bla", untaint($b) )
>
> Mark
Re: Uninitialized value in PerMsgStatus
Posted by Mark Martinec <Ma...@ijs.si>.
2015-01-14 18:06, je Alex Regan napisal
> Hi,
>
> I'm using amavisd-new-2.9.1 and perl-5.18.4 on fedora20 with the svn
> spamassassin snapshot from today, and receive the following message:
>
> Jan 14 11:59:21 mail01 amavis[19431]: (19431-18) _WARN: Use of
> uninitialized value in concatenation (.) or string at
> /usr/share/perl5/vendor_perl/Mail/SpamAssassin/PerMsgStatus.pm line
> 3072, <GEN19> line 16875.
>
> 3071 #loop through and limit to just the domain with a dummy address
> 3072 for (my $i = 0; $i < scalar(@addrs); $i++) {
> 3073 $addrs[$i] =
> 'dummy@'.&Mail::SpamAssassin::Util::uri_to_domain($addrs[$i]);
> 3074 }
>
> This has apparently been going on for some time, not just with today's
> update. I don't see any references to others having this problem, so
> hoped someone could help.
>
> How can I troubleshoot this? Could it be an amavis issue?
>
> Thanks,
> Alex
Looks like someone just recently blindly changed 'return undef'
to a 'return', which breaks calls to such subroutine in a list
context, e.g. foo( untaint($a), "bla", untaint($b) )
Mark