Skip Menu |
Report information
Id: 126981
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: exodist7 [at] gmail.com
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type: unknown
Perl Version: (no value)
Fixed In: (no value)



To: perlbug [...] perl.org
From: Chad Granum <exodist7 [...] gmail.com>
Date: Mon, 21 Dec 2015 10:25:38 -0800
Subject: Possible regression using constant for hash key in perl 5.22+
Download (untitled) / with headers
text/plain 761b
This is a pattern that is used sometimes
 
Show quoted text
sub CONST() { 'some_key' };
my $val = $hash->{+CONST};

This has a couple benefits, one of which is typo protection for hash key lookups. This has worked as far back as I can test (5.8.1). It also still works today in 5.22. However there is a possible bug that appears to have been introduced between 5.20 and 5.22:

Show quoted text
sub CONST() { 'some_key' };
my $val = $hash->{+CONST_TYPO};

The above will fail on perl 5.8->5.20 with the following error:

Show quoted text
 Bareword "CONST_TYPO" not allowed while "strict subs" in use at
 
However on 5.22 it compiles perfectly fine, and also does not issue any warnings or errors at run-time. This is completely new in the 5.22 releases.

I am attaching a sample script to demonstrate it.
Download test.pl
text/x-perl 318b

Message body is not shown because sender requested not to inline it.

To: perl5-porters [...] perl.org
Subject: Re: [perl #126981] Possible regression using constant for hash key in perl 5.22+
From: ilmari [...] ilmari.org (Dagfinn Ilmari Mannsåker)
Date: Mon, 21 Dec 2015 18:44:00 +0000
Download (untitled) / with headers
text/plain 2.1k
Chad Granum (via RT) <perlbug-followup@perl.org> writes: Show quoted text
> sub CONST() { 'some_key' };
>> my $val = $hash->{+CONST_TYPO};
> > > The above will fail on perl 5.8->5.20 with the following error: > > Bareword "CONST_TYPO" not allowed while "strict subs" in use at > > > However on 5.22 it compiles perfectly fine, and also does not issue any > warnings or errors at run-time. This is completely new in the 5.22 releases.
This appears to be due to the multideref optimisation silently eating the const/BARE op before the strict subs checking can get to it. $ perl -MO=Concise -Mstrict -e 'sub CONST() { 'some_key' }; my $h; my $val = $h->{+CONST_TYPO};' 8 <@> leave[1 ref] vKP/REFC ->(end) 1 <0> enter ->2 2 <;> nextstate(main 4 -e:1) v:*,&,{,x*,x&,x$,$ ->3 3 <0> padsv[$h:4,6] vM/LVINTRO ->4 4 <;> nextstate(main 5 -e:1) v:*,&,{,x*,x&,x$,$ ->5 7 <2> sassign vKS/2 ->8 - <1> ex-helem sK/2 ->6 5 <+> multideref($h->{"CONST_TYPO"}) sK/STRICT ->6 - <0> ex-padsv sM/DREFHV ->5 6 <0> padsv[$val:5,6] sRM*/LVINTRO ->7 -e syntax OK If I introduce another check-time error before it, the expected error gets emitted after it: $ perl -MO=Concise -Mstrict -e 'sub CONST() { 'some_key' }; $wat; my $h; my $val = $h->{+CONST_TYPO};' Global symbol "$wat" requires explicit package name (did you forget to declare "my $wat"?) at -e line 1. Bareword "CONST_TYPO" not allowed while "strict subs" in use at -e line 1. -e had compilation errors. e <@> leave[1 ref] KP/REFC ->(end) 1 <0> enter ->2 2 <;> nextstate(main 4 -e:1) :*,&,{,x*,x&,x$,$ ->3 4 <1> rv2sv K/STRICT,1 ->5 3 <$> const(PV "wat") s/ENTERED ->4 5 <;> nextstate(main 4 -e:1) :*,&,{,x*,x&,x$,$ ->6 6 <0> padsv[$h:4,6] ->7 7 <;> nextstate(main 5 -e:1) :*,&,{,x*,x&,x$,$ ->8 d <2> sassign KS/2 ->e b <2> helem K/2 ->c 9 <1> rv2hv K/STRICT,1 ->a 8 <0> padsv[$h:4,6] ->9 a <$> const(PV "CONST_TYPO") /BARE ->b c <0> padsv[$val:5,6] ->d -- "I use RMS as a guide in the same way that a boat captain would use a lighthouse. It's good to know where it is, but you generally don't want to find yourself in the same spot." - Tollef Fog Heen
From: ilmari [...] ilmari.org (Dagfinn Ilmari Mannsåker)
Date: Mon, 21 Dec 2015 19:20:35 +0000
To: perl5-porters [...] perl.org
Subject: Re: [perl #126981] Possible regression using constant for hash key in perl 5.22+
Download (untitled) / with headers
text/plain 655b
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes: Show quoted text
> Chad Granum (via RT) <perlbug-followup@perl.org> writes: >
>> sub CONST() { 'some_key' };
>>> my $val = $hash->{+CONST_TYPO};
>> >> >> The above will fail on perl 5.8->5.20 with the following error: >> >> Bareword "CONST_TYPO" not allowed while "strict subs" in use at >> >> >> However on 5.22 it compiles perfectly fine, and also does not issue any >> warnings or errors at run-time. This is completely new in the 5.22 releases.
> > This appears to be due to the multideref optimisation silently eating > the const/BARE op before the strict subs checking can get to it.
I accidentally a patch:
From d8b194076731858f134f68e484f67b6da1377ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=2E=20Ilmari=20Manns=C3=A5ker?= <ilmari.mannsaker@net-a-porter.com> Date: Mon, 21 Dec 2015 19:11:24 +0000 Subject: [PATCH] [perl #126981] Enforce strict 'subs' in multideref optimisation The code that checks constant keys and turns them into HEKs swallowed the OP_CONST before the strictness checker could get to it, thus allowing barewords when they should not be. --- op.c | 7 +++++++ t/lib/strict/subs | 10 ++++++++++ 2 files changed, 17 insertions(+) diff --git a/op.c b/op.c index 0de303c..bc6b15d 100644 --- a/op.c +++ b/op.c @@ -2343,6 +2343,13 @@ S_check_hash_fields_and_hekify(pTHX_ UNOP *rop, SVOP *key_op) continue; svp = cSVOPx_svp(key_op); + /* make sure it's not a bareword under strict subs */ + if (key_op->op_private & OPpCONST_BARE && + key_op->op_private & OPpCONST_STRICT) + { + no_bareword_allowed((OP*)key_op); + } + /* Make the CONST have a shared SV */ if ( !SvIsCOW_shared_hash(sv = *svp) && SvTYPE(sv) < SVt_PVMG diff --git a/t/lib/strict/subs b/t/lib/strict/subs index 095adee..bad22c6 100644 --- a/t/lib/strict/subs +++ b/t/lib/strict/subs @@ -458,3 +458,13 @@ use strict 'subs'; EXPECT Bareword "FOO" not allowed while "strict subs" in use at - line 3. Execution of - aborted due to compilation errors. +######## +# [perl #126981] Strict subs vs. multideref +sub CONST () { 'some_key' } +my $h; +my $v1 = $h->{+CONST_TYPO}; +use strict 'subs'; +my $v2 = $h->{+CONST_TYPO}; +EXPECT +Bareword "CONST_TYPO" not allowed while "strict subs" in use at - line 6. +Execution of - aborted due to compilation errors. -- 2.6.2
Download (untitled) / with headers
text/plain 285b
-- - Twitter seems more influential [than blogs] in the 'gets reported in the mainstream press' sense at least. - Matt McLeod - That'd be because the content of a tweet is easier to condense down to a mainstream media article. - Calle Dybedahl
Subject: Re: [perl #126981] Possible regression using constant for hash key in perl 5.22+
To: perl5-porters [...] perl.org
Date: Mon, 21 Dec 2015 19:27:08 +0000
From: ilmari [...] ilmari.org (Dagfinn Ilmari Mannsåker)
Download (untitled) / with headers
text/plain 196b
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes: Show quoted text
> I accidentally a patch:
Please disregard the previous patch, it had the wrong email address in the author line. Here's the correct one.
From 0399b344f02eedc9e7cc3fff22acc7c1a6ce917f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Mon, 21 Dec 2015 19:25:32 +0000 Subject: [PATCH] [perl #126981] Enforce strict 'subs' in multideref optimisation The code that checks constant keys and turns them into HEKs swallowed the OP_CONST before the strictness checker could get to it, thus allowing barewords when they should not be. --- op.c | 7 +++++++ t/lib/strict/subs | 10 ++++++++++ 2 files changed, 17 insertions(+) diff --git a/op.c b/op.c index 0de303c..bc6b15d 100644 --- a/op.c +++ b/op.c @@ -2343,6 +2343,13 @@ S_check_hash_fields_and_hekify(pTHX_ UNOP *rop, SVOP *key_op) continue; svp = cSVOPx_svp(key_op); + /* make sure it's not a bareword under strict subs */ + if (key_op->op_private & OPpCONST_BARE && + key_op->op_private & OPpCONST_STRICT) + { + no_bareword_allowed((OP*)key_op); + } + /* Make the CONST have a shared SV */ if ( !SvIsCOW_shared_hash(sv = *svp) && SvTYPE(sv) < SVt_PVMG diff --git a/t/lib/strict/subs b/t/lib/strict/subs index 095adee..bad22c6 100644 --- a/t/lib/strict/subs +++ b/t/lib/strict/subs @@ -458,3 +458,13 @@ use strict 'subs'; EXPECT Bareword "FOO" not allowed while "strict subs" in use at - line 3. Execution of - aborted due to compilation errors. +######## +# [perl #126981] Strict subs vs. multideref +sub CONST () { 'some_key' } +my $h; +my $v1 = $h->{+CONST_TYPO}; +use strict 'subs'; +my $v2 = $h->{+CONST_TYPO}; +EXPECT +Bareword "CONST_TYPO" not allowed while "strict subs" in use at - line 6. +Execution of - aborted due to compilation errors. -- 2.6.2
Download (untitled) / with headers
text/plain 204b
-- "I use RMS as a guide in the same way that a boat captain would use a lighthouse. It's good to know where it is, but you generally don't want to find yourself in the same spot." - Tollef Fog Heen
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.4k
On Mon Dec 21 11:27:42 2015, ilmari wrote: Show quoted text
> ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes: >
> > I accidentally a patch:
> > Please disregard the previous patch, it had the wrong email address in > the author line. Here's the correct one. >
1. It would be nice if someone could run a 'git bisect' to identify the place this regression crept in. 2. Patch appears to have beneficial impact; smoking in smoke-me/jkeenan/ilmari/126981-strict-subs branch. Data: #################### $ perlbrew use perl-5.8.9 $ perl -w -Mstrict -e 'sub CONST() { q|some_key| };my $hash;my $val = $hash->{+CONST_TYPO};' Bareword "CONST_TYPO" not allowed while "strict subs" in use at -e line 1. Execution of -e aborted due to compilation errors. $ perlbrew use perl-5.20.1 $ perl -w -Mstrict -e 'sub CONST() { q|some_key| };my $hash;my $val = $hash->{+CONST_TYPO};' Bareword "CONST_TYPO" not allowed while "strict subs" in use at -e line 1. Execution of -e aborted due to compilation errors. $ perlbrew use perl-5.22.0 $ perl -w -Mstrict -e 'sub CONST() { q|some_key| };my $hash;my $val = $hash->{+CONST_TYPO};' $ [no output] # blead: $ git show | head -1 commit e273c3f6b6604eff1cf509219ad71949b903654a $ ./perl -Ilib -w -Mstrict -e 'sub CONST() { q|some_key| };my $hash;my $val = $hash->{+CONST_TYPO};' $ [no output] # Applying ilmari's patch in smoke-me/jkeenan/ilmari/126981-strict-subs branch restores compile-time failure. ############### -- James E Keenan (jkeenan@cpan.org)
Subject: Re: [perl #126981] Possible regression using constant for hash key in perl 5.22+
Date: Fri, 25 Dec 2015 12:32:59 +0000
To: perlbug-followup [...] perl.org
From: hv [...] crypt.org
CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 262b
"James E Keenan via RT" <perlbug-followup@perl.org> wrote: :1. It would be nice if someone could run a 'git bisect' to identify the place this regression crept in. As suggested by Ilmari's analysis, bisect points at fedf30e1c349130b "Add OP_MULTIDEREF". Hugo
RT-Send-CC: perl5-porters [...] perl.org
Thanks, applied as e1ccd22! -- rjbs
Download (untitled) / with headers
text/plain 252b
Thank you for submitting this report. You have helped make Perl better. With the release of Perl 5.24.0 on May 9, 2016, this and 149 other issues have been resolved. Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org