Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regexp::Grammars broken since 5.17.1 #12787

Closed
p5pRT opened this issue Feb 18, 2013 · 20 comments
Closed

Regexp::Grammars broken since 5.17.1 #12787

p5pRT opened this issue Feb 18, 2013 · 20 comments

Comments

@p5pRT
Copy link

p5pRT commented Feb 18, 2013

Migrated from rt.perl.org#116823 (status was 'resolved')

Searchable as RT116823$

@p5pRT
Copy link
Author

p5pRT commented Feb 18, 2013

From andrew@cleverdomain.org

Created by andrew@cleverdomain.org

This is a bug report for perl from andrew@​cleverdomain.org,
generated with the help of perlbug 1.39 running under perl 5.17.9.

-----------------------------------------------------------------
Regexp​::Grammars fails all of its tests beginning with perl 5.17.1 due to
the reworking of (?{...}) and (??{...}) in that version. The tests all fail
with "Eval-group not allowed at runtime, use re 'eval'" and real code using
Regexp​::Grammars is expected to fail in the same way.

Regexp​::Grammars works by using overload​::constant qr to bless all qr's in
its scope into objects that have overloaded qq, which then does all kinds of
string replacements on the pattern to turn it into one brimming with
(??{...}) constructs. In previous versions of perl, this worked fine
(possibly because all of the overload mojo wasn't considered "runtime"), but
now it's broken.

I'm not sure if this is to be considered a bug in perl, or if the *previous*
behavior was a bug. If this is not a perl bug, how can Regexp​::Grammars fix
itself without requiring its users to pepper their code with use re 'eval'?

Thanks,

Andrew

Perl Info

Flags:
    category=core
    severity=medium

Site configuration information for perl 5.17.9:

Configured by andrew at Sun Feb 17 02:44:05 EST 2013.

Summary of my perl5 (revision 5 version 17 subversion 9) configuration:
  Derived from: 480c67241f1595db244990ae2327660f1eec3602
  Platform:
    osname=linux, osvers=3.5.0-23-generic, archname=x86_64-linux
    uname='linux goron 3.5.0-23-generic #35-ubuntu smp thu jan 24
13:15:40 utc 2013 x86_64 x86_64 x86_64 gnulinux '
    config_args='-de
-Dprefix=/home/andrew/perl5/perlbrew/perls/perl-5.17.8 -Dusedevel
-Aeval:scriptdir=/home/andrew/perl5/perlbrew/perls/perl-5.17.8/bin'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2',
    cppflags='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.7.2', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib/x86_64-linux-gnu /lib/../lib
/usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /usr/lib
    libs=-lnsl -ldl -lm -lcrypt -lutil -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
    libc=, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.15'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib
-fstack-protector'

Locally applied patches:



@INC for perl 5.17.9:
    /home/andrew/perl5/perlbrew/perls/perl-5.17.8/lib/site_perl/5.17.9/x86_64-linux
    /home/andrew/perl5/perlbrew/perls/perl-5.17.8/lib/site_perl/5.17.9
    /home/andrew/perl5/perlbrew/perls/perl-5.17.8/lib/5.17.9/x86_64-linux
    /home/andrew/perl5/perlbrew/perls/perl-5.17.8/lib/5.17.9
    .


Environment for perl 5.17.9:
    HOME=/home/andrew
    LANG=en_US.UTF-8
    LANGUAGE=
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/andrew/perl5/perlbrew/bin:/home/andrew/perl5/perlbrew/perls/perl-5.17.8/bin:/usr/lib/lightdm/lightdm:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
    PERLBREW_BASHRC_VERSION=0.58
    PERLBREW_HOME=/home/andrew/.perlbrew
    PERLBREW_MANPATH=/home/andrew/perl5/perlbrew/perls/perl-5.17.8/man
    PERLBREW_PATH=/home/andrew/perl5/perlbrew/bin:/home/andrew/perl5/perlbrew/perls/perl-5.17.8/bin
    PERLBREW_PERL=perl-5.17.8
    PERLBREW_ROOT=/home/andrew/perl5/perlbrew
    PERLBREW_VERSION=0.58
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From arodland@cpan.org

The commit message for e03b874 makes me
think that Regexp​::Grammars ought to work without changes, since a change
was made for a similar overload-related breakage in Regexp​::Common.

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From [Unknown Contact. See original ticket]

The commit message for e03b874 makes me
think that Regexp​::Grammars ought to work without changes, since a change
was made for a similar overload-related breakage in Regexp​::Common.

@p5pRT
Copy link
Author

p5pRT commented Feb 21, 2013

From @jkeenan

On Sun Feb 17 22​:07​:37 2013, andrew@​cleverdomain.org wrote​:

This is a bug report for perl from andrew@​cleverdomain.org,
generated with the help of perlbug 1.39 running under perl 5.17.9.

-----------------------------------------------------------------
Regexp​::Grammars fails all of its tests beginning with perl 5.17.1 due
to
the reworking of (?{...}) and (??{...}) in that version. The tests all
fail
with "Eval-group not allowed at runtime, use re 'eval'" and real code
using
Regexp​::Grammars is expected to fail in the same way.

Regexp​::Grammars works by using overload​::constant qr to bless all
qr's in
its scope into objects that have overloaded qq, which then does all
kinds of
string replacements on the pattern to turn it into one brimming with
(??{...}) constructs. In previous versions of perl, this worked fine
(possibly because all of the overload mojo wasn't considered
"runtime"), but
now it's broken.

I'm not sure if this is to be considered a bug in perl, or if the
*previous*
behavior was a bug. If this is not a perl bug, how can
Regexp​::Grammars fix
itself without requiring its users to pepper their code with use re
'eval'?

Thanks,

Andrew

We need to investigate the issues which Andrew has raised, as this
sounds like a regression and, hence, a blocker for 5.18.

@p5pRT
Copy link
Author

p5pRT commented Feb 21, 2013

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Mar 3, 2013

From @iabyn

On Sun, Feb 17, 2013 at 10​:07​:37PM -0800, Andrew Rodland wrote​:

Regexp​::Grammars fails all of its tests beginning with perl 5.17.1 due to
the reworking of (?{...}) and (??{...}) in that version. The tests all fail
with "Eval-group not allowed at runtime, use re 'eval'" and real code using
Regexp​::Grammars is expected to fail in the same way.

Regexp​::Grammars works by using overload​::constant qr to bless all qr's in
its scope into objects that have overloaded qq, which then does all kinds of
string replacements on the pattern to turn it into one brimming with
(??{...}) constructs. In previous versions of perl, this worked fine
(possibly because all of the overload mojo wasn't considered "runtime"), but
now it's broken.

I'm not sure if this is to be considered a bug in perl, or if the *previous*
behavior was a bug. If this is not a perl bug, how can Regexp​::Grammars fix
itself without requiring its users to pepper their code with use re 'eval'?

Given that Regexp/Grammars.pm is 6000+ lines of code, is it possible to
supply a short test script (with no external dependencies) that uses
overload​::constant qr and overloaded qq in such a way that demonstrates
the changed behaviour?

Thanks.

--
No man treats a motor car as foolishly as he treats another human being.
When the car will not go, he does not attribute its annoying behaviour to
sin, he does not say, You are a wicked motorcar, and I shall not give you
any more petrol until you go. He attempts to find out what is wrong and
set it right.
  -- Bertrand Russell,
  Has Religion Made Useful Contributions to Civilization?

@p5pRT
Copy link
Author

p5pRT commented Mar 3, 2013

From arodland@cpan.org

On Sat Mar 02 16​:38​:19 2013, davem wrote​:

On Sun, Feb 17, 2013 at 10​:07​:37PM -0800, Andrew Rodland wrote​:

Regexp​::Grammars fails all of its tests beginning with perl 5.17.1
due to
the reworking of (?{...}) and (??{...}) in that version. The tests
all fail
with "Eval-group not allowed at runtime, use re 'eval'" and real
code using
Regexp​::Grammars is expected to fail in the same way.

Regexp​::Grammars works by using overload​::constant qr to bless all
qr's in
its scope into objects that have overloaded qq, which then does all
kinds of
string replacements on the pattern to turn it into one brimming with
(??{...}) constructs. In previous versions of perl, this worked fine
(possibly because all of the overload mojo wasn't considered
"runtime"), but
now it's broken.

I'm not sure if this is to be considered a bug in perl, or if the
*previous*
behavior was a bug. If this is not a perl bug, how can
Regexp​::Grammars fix
itself without requiring its users to pepper their code with use re
'eval'?

Given that Regexp/Grammars.pm is 6000+ lines of code, is it possible
to
supply a short test script (with no external dependencies) that uses
overload​::constant qr and overloaded qq in such a way that
demonstrates
the changed behaviour?

Thanks.

I distilled it down as far as I could in the attached file. Here's a
snippet of "perlbrew exec perl rgtest.pl"​:

perl-5.14.1

ok 1 - It matches
ok 2 - Code ran
1..2

perl-5.16.2

ok 1 - It matches
ok 2 - Code ran
1..2

perl-5.16.2@​ss

ok 1 - It matches
ok 2 - Code ran
1..2

perl-5.17.0

ok 1 - It matches
ok 2 - Code ran
1..2

perl-5.17.1

Eval-group not allowed at runtime, use re 'eval' in regex m/(?{
$RGP​::Success = 1 })foo/ at rgtest.pl line 27.

perl-5.17.8

Eval-group not allowed at runtime, use re 'eval' in regex m/(?{
$RGP​::Success = 1 })foo/ at rgtest.pl line 27.

@p5pRT
Copy link
Author

p5pRT commented Mar 3, 2013

From arodland@cpan.org

rgtest.pl

@p5pRT
Copy link
Author

p5pRT commented Mar 3, 2013

From arodland@cpan.org

On Sat Mar 02 19​:26​:20 2013, arodland wrote​:

I distilled it down as far as I could in the attached file. Here's a
snippet of "perlbrew exec perl rgtest.pl"​:

perl-5.14.1

ok 1 - It matches
ok 2 - Code ran
1..2

perl-5.16.2

ok 1 - It matches
ok 2 - Code ran
1..2

perl-5.16.2@​ss

ok 1 - It matches
ok 2 - Code ran
1..2

perl-5.17.0

ok 1 - It matches
ok 2 - Code ran
1..2

perl-5.17.1

Eval-group not allowed at runtime, use re 'eval' in regex m/(?{
$RGP​::Success = 1 })foo/ at rgtest.pl line 27.

perl-5.17.8

Eval-group not allowed at runtime, use re 'eval' in regex m/(?{
$RGP​::Success = 1 })foo/ at rgtest.pl line 27.

Interestingly, Adding "use re 'eval'" doesn't suppress the error, no
matter which package I add it to (even if I add it in all three).

@p5pRT
Copy link
Author

p5pRT commented Mar 10, 2013

From @iabyn

On Sat, Mar 02, 2013 at 07​:26​:21PM -0800, Andrew Rodland via RT wrote​:

I distilled it down as far as I could in the attached file. Here's a
snippet of "perlbrew exec perl rgtest.pl"​:

Thanks for the reduced test case.

My preliminary analysis is that

1) the test script (and thus presumably Regexp​::Grammars) only worked
because it was inadvertently exploiting a hole in the "use re eval"
mechanism, and that perl 5.17.x is correctly refusing to execute the code;

2) but looking at ways in which Regexp​::Grammars should be modifiable to
"play nicely" aren't in fact working, which indicates that there are bugs
in the new perl mechanism too, which I'll need to look into further, and
presumably fix.

The basic issue is this line in the q{""} overload method​:

  return '(?{ $RGP​::Success = 1 })' . $$re;

this is basically creating an arbitrary string at run-time, which is
subsequently interpreted as as a pattern​: which *should* cause the fatal
error. For example, modifying that line to​:

  return '(?{ $RGP​::Success = 1 })' . $ARGV[0] . $$re;

and when run as​:

  perl5162 -w ~/tmp/rgtest.pl '(?{ system "echo hacked"})'

you get​:

  hacked
  ok 1 - It matches
  ok 2 - Code ran
  1..2

which shows that perl is executing code in arbitrary run-time strings
without 'use re eval' in scope.

The basic rule for 5.17.1 onwards is that literal text containing (?{})
that is within a pattern doesn't need 'use re eval'; everything else
should. This is in many ways a relaxing of the constraints from earlier
perls​: this now works without 'use re eval', even though it is a run-time
pattern​:

  $x = "foo"; $x =~ /(?{ print "boo\n" })$x/;

while other things that were lax have been stiffened.

However, I would expect something like the following *should* work and be
safe, but it doesn't​:

  use re 'eval'; return qr/(?{ $RGP​::Success = 1 })$$re/;

And as you pointed out, a 'use re eval' even in the scope of the main
qr/foo/ doesn't turn off the fatal error. So there's definitely something
there needing more work.

--
You never really learn to swear until you learn to drive.

@p5pRT
Copy link
Author

p5pRT commented Mar 27, 2013

From zefram@fysh.org

Dave Mitchell wrote​:

1) the test script (and thus presumably Regexp​::Grammars) only worked
because it was inadvertently exploiting a hole in the "use re eval"
mechanism, and that perl 5.17.x is correctly refusing to execute the code;

The qr constant overloading behaves in a rather strange way, compared to
operator overloading. After the R​:G​:Precursor object has been built up
by the constant-overload function, it gets stringified, invoking the ""
operator overload on R​:G​:Precursor. The problem is that the resulting
string gets compiled to a regexp object by the core, not by the module
or the calling program​: where the re-eval setting matters, there's no
way to set it.

You'd think you could avoid this problem by making R​:G return a compiled
regexp somewhere instead of a string​: it can set re-eval for its explicit
compilation. But the core is apparently quite insistent on reducing
the qr-constant to a string. If the constant-overload function returns
a compiled regexp, that gets stringified. If the "" operator overload
returns a compiled regexp, that gets stringified. If the "" operator
overload returns a ref to an object with more operator overloading,
that gets stringified. qr operator overloading never gets invoked.

I reckon the qr constant overload should be allowed to return a compiled
regexp, to be used as-is rather than stringified.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Mar 27, 2013

From @iabyn

On Wed, Mar 27, 2013 at 10​:58​:23AM +0000, Zefram wrote​:

I reckon the qr constant overload should be allowed to return a compiled
regexp, to be used as-is rather than stringified.

Yes, that's exactly what I've been working on for the last week.

--
No matter how many dust sheets you use, you will get paint on the carpet.

@p5pRT
Copy link
Author

p5pRT commented Apr 11, 2013

From @iabyn

On Sun, Mar 10, 2013 at 12​:48​:16AM +0000, Dave Mitchell wrote​:

However, I would expect something like the following *should* work and be
safe, but it doesn't​:

use re 'eval'; return qr/\(?\{ $RGP​::Success = 1 \}\)$$re/;

And as you pointed out, a 'use re eval' even in the scope of the main
qr/foo/ doesn't turn off the fatal error. So there's definitely something
there needing more work.

The branch smoke-me/davem/re_overload, currently being smoked and shortly
to be merged into blead, should fix this.

If the following change is made to your test script​:

Inline Patch
--- /tmp/rgtest.pl.orig	2013-04-11 10:28:53.879421869 +0100
+++ /tmp/rgtest.pl	2013-04-11 10:31:57.510778815 +0100
@@ -18,7 +18,7 @@
   package RGP;
   use overload q{""} => sub {
     my $re = shift;
-    return '(?{ $RGP::Success = 1 })' . $$re;
+    return qr/(?{ $RGP::Success = 1 })$$re/;
   }, fallback => 1;
 }
 
then it passes both tests when using my new branch. To make it still work on 5\.16\.x and earlier\, you also need a use re eval​:

- return '(?{ $RGP​::Success = 1 })' . $$re;
+ use re 'eval';
+ return qr/(?{ $RGP​::Success = 1 })$$re/;

which is harmless, but unnecessary after my fix to blead.

I haven't tried fixing up Regex​::Grammars to see if it works, since it's
too complex for a cursory glance and fix-up. I see lots of places in the
code where it returns strings containing '(?{', but I don't know how
practical it would be to convert all those to return qr//'s instead.

--
Gravity is just a theory; teach Intelligent Falling in our schools!
  http​://www.theonion.com/content/node/39512

@p5pRT
Copy link
Author

p5pRT commented Apr 12, 2013

From zefram@fysh.org

Dave Mitchell wrote​:

I haven't tried fixing up Regex​::Grammars to see if it works, since it's
too complex for a cursory glance and fix-up. I see lots of places in the
code where it returns strings containing '(?{', but I don't know how
practical it would be to convert all those to return qr//'s instead.

It only needs a qr// once, at the top level.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Apr 12, 2013

From @iabyn

On Thu, Apr 11, 2013 at 10​:39​:26AM +0100, Dave Mitchell wrote​:

On Sun, Mar 10, 2013 at 12​:48​:16AM +0000, Dave Mitchell wrote​:

However, I would expect something like the following *should* work and be
safe, but it doesn't​:

use re 'eval'; return qr/\(?\{ $RGP​::Success = 1 \}\)$$re/;

And as you pointed out, a 'use re eval' even in the scope of the main
qr/foo/ doesn't turn off the fatal error. So there's definitely something
there needing more work.

The branch smoke-me/davem/re_overload, currently being smoked and shortly
to be merged into blead, should fix this.

And now in blead with commit

  e501306

--
My get-up-and-go just got up and went.

@p5pRT
Copy link
Author

p5pRT commented Apr 12, 2013

@iabyn - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this as completed Apr 12, 2013
@p5pRT
Copy link
Author

p5pRT commented Apr 13, 2013

From @rjbs

* Dave Mitchell <davem@​iabyn.com> [2013-04-12T10​:57​:15]

And now in blead with commit

e501306eca0fea1cc9fc53e2eb024ad37e85ce59

I'm delighted, thanks Dave!

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Oct 16, 2013

From @hroncok

Why is this marked as solved? I cannot run Regexp​::Grammars in 5.18.
Even if I backport all mentioned commits​:

commit c3923c3
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Tue Aug 6 16​:34​:50 2013 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Tue Aug 6 16​:44​:12 2013 +0100

  reparse compile-time /(?{})/ in right scope

<http​://www.nntp.perl.org/group/perl.perl5.porters/2013/07/msg205393.htm
l>​:

commit c30fc27
Author​: David Mitchell <davem@​iabyn.com>
Date​: Wed Jul 31 22​:41​:17 2013 +0100

  Handle /[#]/ and /[(?#]/ with code blocks

Is there a fix somewhere present and I just don't see it? Thanks

@p5pRT
Copy link
Author

p5pRT commented Oct 16, 2013

From [Unknown Contact. See original ticket]

Why is this marked as solved? I cannot run Regexp​::Grammars in 5.18.
Even if I backport all mentioned commits​:

commit c3923c3
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Tue Aug 6 16​:34​:50 2013 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Tue Aug 6 16​:44​:12 2013 +0100

  reparse compile-time /(?{})/ in right scope

<http​://www.nntp.perl.org/group/perl.perl5.porters/2013/07/msg205393.htm
l>​:

commit c30fc27
Author​: David Mitchell <davem@​iabyn.com>
Date​: Wed Jul 31 22​:41​:17 2013 +0100

  Handle /[#]/ and /[(?#]/ with code blocks

Is there a fix somewhere present and I just don't see it? Thanks

@p5pRT
Copy link
Author

p5pRT commented Oct 18, 2013

From @Leont

On Wed, Oct 16, 2013 at 4​:37 PM, Miro HronÄ�ok via RT <
perlbug-comment@​perl.org> wrote​:

Why is this marked as solved? I cannot run Regexp​::Grammars in 5.18.
Even if I backport all mentioned commits​:

commit c3923c3
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Tue Aug 6 16​:34​:50 2013 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Tue Aug 6 16​:44​:12 2013 +0100

reparse compile\-time /\(?\{\}\)/ in right scope

<http​://www.nntp.perl.org/group/perl.perl5.porters/2013/07/msg205393.htm
l>​:

commit c30fc27
Author​: David Mitchell <davem@​iabyn.com>
Date​: Wed Jul 31 22​:41​:17 2013 +0100

Handle /\[\#\]/ and /\[\(?\#\]/ with code blocks

Is there a fix somewhere present and I just don't see it? Thanks

CPAN Testers is all green on 5.18.1, as well as 5.19.2+. Are you sure
you're not accidentally trying to run this on 5.18.0?

Leon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant