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

Possible regression using constant for hash key in perl 5.22+ #15099

Closed
p5pRT opened this issue Dec 21, 2015 · 16 comments
Closed

Possible regression using constant for hash key in perl 5.22+ #15099

p5pRT opened this issue Dec 21, 2015 · 16 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Dec 21, 2015

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

Searchable as RT126981$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2015

From @exodist

This is a pattern that is used sometimes

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​:

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.

I am attaching a sample script to demonstrate it.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2015

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2015

From @ilmari

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.

$ 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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2015

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2015

From @ilmari

ilmari@​ilmari.org (Dagfinn Ilmari Mannsåker) writes​:

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​:

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2015

From @ilmari

0001-perl-126981-Enforce-strict-subs-in-multideref-optimi.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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2015

From @ilmari

--
- 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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2015

From @ilmari

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.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2015

From @ilmari

0001-perl-126981-Enforce-strict-subs-in-multideref-optimi.patch
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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2015

From @ilmari

--
"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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 24, 2015

From @jkeenan

On Mon Dec 21 11​:27​:42 2015, ilmari wrote​:

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 e273c3f
$ ./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)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 25, 2015

From @hvds

"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 fedf30e
"Add OP_MULTIDEREF".

Hugo

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 17, 2016

From @rjbs

Thanks, applied as e1ccd22!

--
rjbs

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 17, 2016

@rjbs - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 13, 2016

From @khwilliamson

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 13, 2016

@khwilliamson - Status changed from 'pending release' to 'resolved'

@p5pRT p5pRT closed this May 13, 2016
@p5pRT p5pRT added the Severity Low label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.