Bug 7591

Summary: failure on idn_dots.t
Product: Spamassassin Reporter: Kevin A. McGrail <kmcgrail>
Component: Regression TestsAssignee: SpamAssassin Developer Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal CC: apache, billcole, giovanni, kmcgrail, nicolas.rochelemagne
Priority: P2    
Version: 3.4 SVN branch   
Target Milestone: Undefined   
Hardware: PC   
OS: Windows NT   
Whiteboard:
Attachments: Patch to reversethe fast untaint in Util.pm
Test if get_uri_list and spamassassin script agree on number of URIs

Description Kevin A. McGrail 2018-06-26 15:00:27 UTC
Created attachment 5577 [details]
Patch to reversethe fast untaint in Util.pm

3.4 fails passing tests for me with the idn_dots.t 

If you modify the t/idn_dots.t to print the uri list from the generated message in the test, it fails in 3.4 but passes in Trunk and in the 3.4.1 release.  

When it prints the URI list for me, it's missing the utf1.example.com item.

Interestingly, it passes with 3 or less URIs and fails with 4 or more.

Giovanni Becchis, Bill Cole and myself have recreated it under different scenarios but the key point seems to be Centos/Redhat

 - Confirmed problems with Centos/RHEL including:
     Centos 7.4.1708 and stock perl 5.16.3 FAILS subtest 4
     Centos 7.5.1804  with stock 5.16.3 also fails
 - Centos 7.5.1804 with custom compiled perl 5.26.2 does not fail.
 - Works on other things such as Ubuntu & MacOS 10.6.8 and 10.11.6 with perl 5.26.2
 - Running the test multiple times sometimes gives different results.


Steps to Recreate:

On a CentOS 7 box
svn checkout https://svn.apache.org/repos/asf/spamassassin/branches/3.4
cd 3.4
perl Makefile.PL
make
prove -v t/idn_dots.t


Other Notes:

- Adding "use utf8;" to t/idn_dots.t fixes the problem but it is not clear why.  We consider this hiding the issue.

- Trunk has the issue but it is more hidden.  Bill Cole will make a test for it.

- Adding more text to the t/idn_dots.t files (my $string = "http://utf$i" . $delim . "example" . $delim . "com";) to (my $string = "an url: http://utf$i" . $delim . "example" . $delim . "com";) makes things fail with less URLs.

- When using the perl debugger on seems to fix it


Solution #1:
  - reverting the fast untaint patch work in util.pm on bug 7496 appears to work
  - Confirm after the hidden test case for trunk is completed by Bill

Solution #2:
  - Also using a newer perl appears to work.


-- NOTE: Custom Perl Notes

rm -f config.sh Policy.sh
sh Configure -des -Dprefix=~/usr/local/perl-5.26 -Dusedl -Acflags='-fPID -DPIC -m64'
make -j4
make install
rm ~/.cpan -rf
~/usr/local/perl-5.26/bin/cpan install HTML::Parser Net::DNS NetAddr::IP

~/usr/local/perl-5.26/bin/perl Makefile.PL
svn checkout https://svn.apache.org/repos/asf/spamassassin/branches/3.4
cd 3.4
~/usr/local/perl-5.26/bin/perl Makefile.PL
make
~/usr/local/perl-5.26/bin/prove -v t/idn_dots.t
Comment 1 Bill Cole 2018-06-26 17:15:53 UTC
Created attachment 5578 [details]
Test if get_uri_list and spamassassin script agree on number of URIs

Counts the URIs in 6 simple messages 3 ways: direct grep, get_uri_list(), and spamassassin -D
Comment 2 Giovanni Bechis 2018-07-03 12:38:17 UTC
Some more info that could explain the bug in RH bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1597565
Comment 3 Kevin A. McGrail 2018-07-03 12:50:50 UTC
Thanks Giovanni.  Glad you were able to confirm that a custom compiled perl resolves the issue too.  With the removal of the untaint code that was causing the failure, I'm using Bill's tests to see if I think we can release things.  But it is good to open a bug with RH as it points to some issue with their compilation is possible.
Comment 4 Giovanni Bechis 2018-07-11 18:16:10 UTC
Test passes on CentOS 7.5 with perl 5.16 built from src.rpm (with all RH patches) but with -D_FORTIFY_SOURCE=0.
This is definitely a Perl bug that has been fixed in later Perl versions.
IMHO the best we can do is revert the offending line of code.
Comment 5 Kevin A. McGrail 2018-07-11 18:55:04 UTC
Thanks for cross referencing to RH.  Any word if RH is able to fix it?  I think this clears the blocker with the code removed.
Comment 6 Giovanni Bechis 2018-07-11 22:02:13 UTC
RH asked me to recreate the problem with a snippet,
without using all SA codebase.
I tried a bit but I failed.
Comment 7 Kevin A. McGrail 2018-08-19 16:12:30 UTC
Reversing the fast untaint patch and considering closed.  It's a bug in perl but getting a snippet for RH so they can backport isn't something the project can do.
Comment 8 Kevin A. McGrail 2018-08-19 16:13:29 UTC
Committed revision 1838387.
Committed revision 1838388.
Comment 9 Nicolas R. 2018-10-08 16:26:21 UTC
we could also consider checking the perl version and if $] is greater than for example 5.025 then we could use this fast untaint technique?
Comment 10 Henrik Krohns 2018-10-08 16:49:52 UTC
The problem is accepting "performance" patches with zero proof of actually delivering. The patch that included this "faster untaint" had several other changes that actually worsened performance.