perl5 icon indicating copy to clipboard operation
perl5 copied to clipboard

a02b8151f9 causes test failures in Module::Extract::VERSION

Open jkeenan opened this issue 3 years ago • 4 comments

A commit from several months back has caused test failures in CPAN distribution Module-Extract-VERSION (@briandfoy).

Sample CPANtesters failure report: http://www.cpantesters.org/cpan/report/c464ec32-111c-11ed-948b-af529faf9438

The failures look like this:

$ bleadprove -vb t/corpus.t
t/corpus.t .. 
ok 1 - use Module::Extract::VERSION;
ok 2 - Module::Extract::VERSION->can('parse_version_safely')
ok 3 - Corpus file [ corpus/Dotted_5_12.pm ] exists
ok 4 - Works for Dotted_5_12.pm
ok 5 - Corpus file [ corpus/Dotted_5_14_braces.pm ] exists
not ok 6 - Works for Dotted_5_14_braces.pm

#   Failed test 'Works for Dotted_5_14_braces.pm'
#   at t/corpus.t line 36.
#          got: undef
#     expected: 'v0.10.01'
ok 7 - Corpus file [ corpus/Easy.pm ] exists
not ok 8 - Works for Easy.pm

#   Failed test 'Works for Easy.pm'
#   at t/corpus.t line 36.
#          got: undef
#     expected: '3.01'
ok 9 - Corpus file [ corpus/Easy_5_12.pm ] exists
not ok 10 - Works for Easy_5_12.pm

#   Failed test 'Works for Easy_5_12.pm'
#   at t/corpus.t line 36.
#          got: undef
#     expected: '3.01'
ok 11 - Corpus file [ corpus/Easy_5_14_braces.pm ] exists
not ok 12 - Works for Easy_5_14_braces.pm

#   Failed test 'Works for Easy_5_14_braces.pm'
#   at t/corpus.t line 36.
#          got: undef
#     expected: '3.01'
ok 13 - Corpus file [ corpus/RCSKeywords.pm ] exists
not ok 14 - Works for RCSKeywords.pm

#   Failed test 'Works for RCSKeywords.pm'
#   at t/corpus.t line 36.
#          got: undef
#     expected: '1.23'
ok 15 - Corpus file [ corpus/ToTk.pm ] exists
ok 16 - Works for ToTk.pm
ok 17 - Corpus file [ corpus/Underscore.pm ] exists
not ok 18 - Works for Underscore.pm

#   Failed test 'Works for Underscore.pm'
#   at t/corpus.t line 36.
#          got: undef
#     expected: '0.10_01'
1..18
# Looks like you failed 6 tests of 18.
Dubious, test returned 6 (wstat 1536, 0x600)
Failed 6/18 subtests 

Test Summary Report
-------------------
t/corpus.t (Wstat: 1536 (exited 6) Tests: 18 Failed: 6)
  Failed tests:  6, 8, 10, 12, 14, 18
  Non-zero exit status: 6
Files=1, Tests=18,  0 wallclock secs ( 0.02 usr  0.02 sys +  0.11 cusr  0.02 csys =  0.17 CPU)
Result: FAIL

Bisection points to a02b8151f9b69201233f9ca5774db280c34684de.

a02b8151f9b69201233f9ca5774db280c34684de is the first bad commit
commit a02b8151f9b69201233f9ca5774db280c34684de
Author: James Raspass <[email protected]>
Date:   Sat Jun 11 23:33:31 2022 +0100
Commit:     Tony Cook <[email protected]>
CommitDate: Tue Jul 5 11:17:31 2022 +1000

    Add builtin::is_tainted
    
    Also tweak the implementation of the other two boolean builtins (is_bool
    & is_weak) to be slightly more efficient.

Reference: https://github.com/Perl/perl5/pull/19854

@JRaspass, @tonycoz, can you take a look?

jkeenan avatar Sep 05 '22 12:09 jkeenan

So add the following patch to t/corpus.t:

--- a/t/corpus.t
+++ b/t/corpus.t
@@ -33,6 +33,8 @@ foreach my $file ( sort keys %Corpus )
        my $version =
                eval{ Module::Extract::VERSION->parse_version_safely( $path ) };
 
+       diag $@ if $@;
+
        is( $version, $Corpus{$file}, "Works for $file" );
 
        }

Reports this error:

# Undefined subroutine &version::(<=> called at /home/jraspass/perl5/perlbrew/perls/perl-5.37.3/lib/5.37.3/Safe.pm line 176, <$fh> line 4.

Line 176 of Safe does this:

Opcode::_safe_pkg_prep($obj->{Root}) if($Opcode::VERSION > 1.04);

And because #19854 converted Opcode to have a modern version syntax:

--- a/ext/Opcode/Opcode.pm
+++ b/ext/Opcode/Opcode.pm
@@ -1,31 +1,21 @@
-package Opcode;
-
-use 5.006_001;
+package Opcode 1.59;
 
 use strict;
 
-our($VERSION, @ISA, @EXPORT_OK);
-
-$VERSION = "1.58";
-

So what I think is happening is $Opcode::VERSION is now a real version object (whereas before it was a string) so the overloaded numeric comparison is being called but that is a function which isn't in Safe's whitelist so it goes boom. Does that seem right to anyone else? Pretty new to Safe myself.

So I think we can either go back to the legacy way of specifying a version, but that seems unfortunate, or change Safe to be less fragile, maybe just hoisting the version check to a constant for compile time would be enough? I presume that would run before Safe starts whitelisting functions.

JRaspass avatar Sep 05 '22 19:09 JRaspass

Oh I guess Safe is dual-life which is why it's checking Opcode version in the first place, hmm.

JRaspass avatar Sep 05 '22 19:09 JRaspass

So add the following patch to t/corpus.t:

@JRaspass, when we get an issue that reports breakage of CPAN tests associated with a commit in Perl blead (so-called BBC tickets), we have to determine what part of the breakage is associated with flaws in blead, what part is associated with flaws in CPAN, and what is the balance of the two. That means there may need to be corrections in both Perl blead and on CPAN -- but P5P needs to do its part first, and then someone needs to submit a patch to CPAN as needed. That's why on such tickets we generally ask both the Author and Committer to have a look.

jkeenan avatar Sep 06 '22 17:09 jkeenan

So I think we can either go back to the legacy way of specifying a version, but that seems unfortunate, or change Safe to be less fragile, maybe just hoisting the version check to a constant for compile time would be enough? I presume that would run before Safe starts whitelisting functions.

This seems like a reasonable analysis, and if I change the Opcode version to use our $VERSION = ... the M::E::VERSION tests pass.

I'm not that familiar with Safe, but it looks like the &version::(<=> sub is being shared with the safe root (Safe.pm:174), so I don't see why it's crashing (and that code shouldn't be happening within the Safe compartment anyway).

tonycoz avatar Sep 07 '22 01:09 tonycoz

Safe is sharing &version::(<=>, but then Module::Extract::VERSION is itself sharing *version::. This is at least in part what leads to the failure. I've filed a PR to remove that: briandfoy/module-extract-version#16

haarg avatar Oct 23 '22 16:10 haarg

I've merged https://github.com/briandfoy/module-extract-version/pull/16 and that seems to pass all the tests. I'll bump the version and see what CPAN Testers says.

briandfoy avatar Oct 25 '22 12:10 briandfoy

Module-Extract-VERSION-1.116 has been released and is passing tests against blead. Closing this ticket. Thanks.

jkeenan avatar Oct 25 '22 18:10 jkeenan