splice doesn't honor Internals::SvREADONLY. #15923
Comments
From peter@morch.comCreated by peter@morch.comSee: Calling splice() on Immutable Arrays or Bug #120130 for Const-Fast: splice doesn't honor Internals::SvREADONLY a tiny script to reproduce: #!/usr/bin/perl -w Perl Info |
From @jkeenanOn Wed, 15 Mar 2017 02:31:23 GMT, pmorch wrote:
First, the obligatory disclaimers from lib/Internals.pod: ##### In theory these routines were not and are not intended to be used outside of the perl core, and are subject to change and removal at any time. In practice people have come to depend on these over the years, despite being historically undocumented, so we will provide some level of .... =item SvREADONLY(THING, [, $value]) Set or get whether a variable is readonly or not. Exactly what the readonly flag means depend on the type of the variable affected and the You are strongly discouraged from using this function directly. It is used by various core modules, like C<Hash::Util>, and the C<constant> pragma to implement higher-level behavior which should be used instead. See the core implementation for the exact meaning of the readonly flag for each internal variable type. Given that documentation, the fact that 'splice' does not honor Internals::SvREADONLY is more of an unimplemented feature than a bug. The fact that CPAN distro Const-Fast relies upon Internals::SvREADONLY is irrelevant. Unless we can document a need for the Perl 5 core distribution to have 'splice' honor Internals::SvREADONLY, I don't think we should do it, as that would only encourage people to include it in non-core code. Thank you very much. |
|
The RT System itself - Status changed from 'new' to 'open' |
From @arcJames E Keenan via RT <perlbug-followup@perl.org> wrote:
I think this is definitely a bug that should be fixed. splice @{^CAPTURE}, 0, 0, "oops"; just as this does: unshift @{^CAPTURE}, "oops"; There's an additional oddity in this area: this code silently does nothing: push @readonly_array, (); but this code throws an exception: unshift @readonly_array, (); Given that pp_push contains specific code to permit push-of-nothing Sawyer, is this OK to merge before 5.26.0? It eliminates a -- |
From @arc0001-RT-131000-splice-doesn-t-honour-read-only-flag.patchFrom 862877e9b3a7a193452e7ae52a8a7bb8dc244568 Mon Sep 17 00:00:00 2001
From: Aaron Crane <arc@cpan.org>
Date: Thu, 16 Mar 2017 12:33:59 +0000
Subject: [PATCH] RT#131000: splice doesn't honour read-only flag
The push and unshift builtins were correctly throwing a "Modification of a
read-only value attempted" exception when modifying a read-only array, but
splice was silently modifying the array.
However, push silently accepted a push of no elements onto an array, whereas
unshift threw an exception in that situation.
After this commit, all three now have the same behaviour: they throw an
exception when the array is read-only and would be modified, but the
exception is suppressed if no real change is being made.
---
pp.c | 5 ++++-
t/op/push.t | 14 +++++++++++++-
t/op/splice.t | 23 +++++++++++++++++++++++
t/op/unshift.t | 13 ++++++++++++-
4 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/pp.c b/pp.c
index a640995e31..9c9bab2a1a 100644
--- a/pp.c
+++ b/pp.c
@@ -5327,6 +5327,8 @@ PP(pp_splice)
/* At this point, MARK .. SP-1 is our new LIST */
newlen = SP - MARK;
+ if ((newlen > 0 || length > 0) && SvREADONLY(ary))
+ Perl_croak_no_modify();
diff = newlen - length;
if (newlen && !AvREAL(ary) && AvREIFY(ary))
av_reify(ary);
@@ -5558,7 +5560,8 @@ PP(pp_unshift)
U16 old_delaymagic = PL_delaymagic;
SSize_t i = 0;
- av_unshift(ary, SP - MARK);
+ if (SP > MARK)
+ av_unshift(ary, SP - MARK);
PL_delaymagic = DM_DELAY;
while (MARK < SP) {
SV * const sv = newSVsv(*++MARK);
diff --git a/t/op/push.t b/t/op/push.t
index c94c91953f..6da16b2a52 100644
--- a/t/op/push.t
+++ b/t/op/push.t
@@ -20,7 +20,7 @@ BEGIN {
-4, 4 5 6 7, 0 1 2 3
EOF
-plan tests => 8 + @tests*2;
+plan tests => 10 + @tests*2;
die "blech" unless @tests;
@x = (1,2,3);
@@ -71,4 +71,16 @@ foreach $line (@tests) {
is(join(':',@x), join(':',@leave), "left: @x == @leave");
}
+{
+ local $@;
+ my @readonly_array = 10..11;
+ Internals::SvREADONLY(@readonly_array, 1);
+ eval { push @readonly_array, () };
+ is $@, '', "can push empty list onto readonly array";
+
+ eval { push @readonly_array, 9 };
+ like $@, qr/^Modification of a read-only value/,
+ "exception for pushing onto readonly array";
+}
+
1; # this file is require'd by lib/tie-stdpush.t
diff --git a/t/op/splice.t b/t/op/splice.t
index 7ad49db2ba..2d82bb1a3a 100644
--- a/t/op/splice.t
+++ b/t/op/splice.t
@@ -98,4 +98,27 @@ $#a++;
is sprintf("%s", splice @a, 0, 1, undef), "",
'splice handles nonexistent elems when array len stays the same';
+{
+ local $@;
+ my @readonly_array = 10..11;
+ Internals::SvREADONLY(@readonly_array, 1);
+ eval { splice @readonly_array, 1, 0, () };
+ is $@, '', "can splice empty list into readonly array without deletion";
+
+ # RT#131000 "splice doesn't honor Internals::SvREADONLY"
+ eval { splice @readonly_array, 1, 1 };
+ like $@, qr/^Modification of a read-only value/,
+ "exception for splicing into readonly array (delete but no insert)";
+
+ $@ = '';
+ eval { splice @readonly_array, 1, 0, 9 };
+ like $@, qr/^Modification of a read-only value/,
+ "exception for splicing into readonly array (insert but no delete)";
+
+ $@ = '';
+ eval { splice @readonly_array, 1, 1, 9 };
+ like $@, qr/^Modification of a read-only value/,
+ "exception for splicing into readonly array (delete and insert)";
+}
+
done_testing;
diff --git a/t/op/unshift.t b/t/op/unshift.t
index 66fd0ff86a..4823aab0e1 100644
--- a/t/op/unshift.t
+++ b/t/op/unshift.t
@@ -5,7 +5,7 @@ BEGIN {
require "./test.pl";
}
-plan(18);
+plan(20);
@array = (1, 2, 3);
@@ -68,3 +68,14 @@ is(join(' ',@alpha), 's t u v w x y z', 'void unshift array');
unshift (@alpha, @bet, @gimel);
is(join(' ',@alpha), 'q r s t u v w x y z', 'void unshift arrays');
+{
+ local $@;
+ my @readonly_array = 10..11;
+ Internals::SvREADONLY(@readonly_array, 1);
+ eval { unshift @readonly_array, () };
+ is $@, "", "can unshift empty list onto readonly array";
+
+ eval { unshift @readonly_array, 9 };
+ like $@, qr/^Modification of a read-only value/,
+ "exception for unshifting onto readonly array";
+}
--
2.11.0
|
From zefram@fysh.orgAaron Crane wrote:
It would be better to make this consistent in the other direction. -zefram |
From @jkeenanOn Thu, 16 Mar 2017 12:48:46 GMT, arc wrote:
Even if we were to agree that it is a bug, it's clearly not a regression. So, given that we're in code freeze, I don't think it's eligible for 5.26.0. (Also, Zefram's counter-argument merits consideration.) Thank you very much. -- |
From @LeontOn Thu, Mar 16, 2017 at 1:47 PM, Aaron Crane <arc@cpan.org> wrote:
Agreed. This is clearly a bug.
That is a surprising inconsistency. I don't think it's deliberate
I can't think of any technical reason not to fix the splice thing Leon |
From @arcLeon Timmermans <fawaka@gmail.com> wrote:
I've attached a replacement patch that fixes the bug in splice The bug fix amounts to the addition of the following two lines of code if (SvREADONLY(ary)) It seems to me that we wouldn't be serving Perl's users well to -- |
From @arc0001-v2-RT-131000-splice-doesn-t-honour-read-only-flag.patchFrom d80437041d3ad8380df5512d4a41b2312eda5e3e Mon Sep 17 00:00:00 2001
From: Aaron Crane <arc@cpan.org>
Date: Thu, 16 Mar 2017 12:33:59 +0000
Subject: [PATCH] RT#131000: splice doesn't honour read-only flag
The push and unshift builtins were correctly throwing a "Modification of a
read-only value attempted" exception when modifying a read-only array, but
splice was silently modifying the array. This commit adds tests that all
three builtins throw such an exception.
One discrepancy between the three remains: push silently accepted a push of
no elements onto an array, whereas unshift throws an exception in that
situation. This behaviour in push dates back to at least Perl 5.8; I can't
easily test earlier versions of Perl.
I believe it would be better for these three builtins to behave consistently
when asked to perform a non-modifying modification to a read-only array.
However, since we're currently in code freeze, this commit is limited to a
fix of the unambiguous bug in splice, and tests of the existing behaviour in
push and unshift; and splice throws an exception in all cases, even those
where push would throw no exception, on the grounds that it's easier to
relax a constraint than to introduce a new one.
---
pp.c | 3 +++
t/op/push.t | 14 +++++++++++++-
t/op/splice.t | 9 +++++++++
t/op/unshift.t | 10 +++++++++-
4 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/pp.c b/pp.c
index a640995e31..cd197e42a1 100644
--- a/pp.c
+++ b/pp.c
@@ -5288,6 +5288,9 @@ PP(pp_splice)
sp - mark);
}
+ if (SvREADONLY(ary))
+ Perl_croak_no_modify();
+
SP++;
if (++MARK < SP) {
diff --git a/t/op/push.t b/t/op/push.t
index c94c91953f..6da16b2a52 100644
--- a/t/op/push.t
+++ b/t/op/push.t
@@ -20,7 +20,7 @@ BEGIN {
-4, 4 5 6 7, 0 1 2 3
EOF
-plan tests => 8 + @tests*2;
+plan tests => 10 + @tests*2;
die "blech" unless @tests;
@x = (1,2,3);
@@ -71,4 +71,16 @@ foreach $line (@tests) {
is(join(':',@x), join(':',@leave), "left: @x == @leave");
}
+{
+ local $@;
+ my @readonly_array = 10..11;
+ Internals::SvREADONLY(@readonly_array, 1);
+ eval { push @readonly_array, () };
+ is $@, '', "can push empty list onto readonly array";
+
+ eval { push @readonly_array, 9 };
+ like $@, qr/^Modification of a read-only value/,
+ "exception for pushing onto readonly array";
+}
+
1; # this file is require'd by lib/tie-stdpush.t
diff --git a/t/op/splice.t b/t/op/splice.t
index 7ad49db2ba..238c960257 100644
--- a/t/op/splice.t
+++ b/t/op/splice.t
@@ -98,4 +98,13 @@ $#a++;
is sprintf("%s", splice @a, 0, 1, undef), "",
'splice handles nonexistent elems when array len stays the same';
+{
+ local $@;
+ my @readonly_array = 10..11;
+ Internals::SvREADONLY(@readonly_array, 1);
+ eval { splice @readonly_array, 1, 0, () };
+ like $@, qr/^Modification of a read-only value/,
+ "exception for splicing into readonly array";
+}
+
done_testing;
diff --git a/t/op/unshift.t b/t/op/unshift.t
index 66fd0ff86a..d972637e18 100644
--- a/t/op/unshift.t
+++ b/t/op/unshift.t
@@ -5,7 +5,7 @@ BEGIN {
require "./test.pl";
}
-plan(18);
+plan(19);
@array = (1, 2, 3);
@@ -68,3 +68,11 @@ is(join(' ',@alpha), 's t u v w x y z', 'void unshift array');
unshift (@alpha, @bet, @gimel);
is(join(' ',@alpha), 'q r s t u v w x y z', 'void unshift arrays');
+{
+ local $@;
+ my @readonly_array = 10..11;
+ Internals::SvREADONLY(@readonly_array, 1);
+ eval { unshift @readonly_array, () };
+ like $@, qr/^Modification of a read-only value/,
+ "exception for unshifting onto readonly array";
+}
--
2.11.0
|
From @iabynOn Fri, Mar 17, 2017 at 12:07:03PM +0000, Aaron Crane wrote:
Except that it could make code which currently works fail. It's probably I think it should wait for 5.27.x. -- |
From @xsawyerxOn 03/17/2017 04:09 PM, Dave Mitchell wrote:
I had expressed the same opinion on IRC. (I'm still open to continuing discussing this, though, but on Monday a |
From @jkeenanOn Sat, 18 Mar 2017 17:38:07 GMT, xsawyerx@gmail.com wrote:
With perl-5.26.0 having been released, this ticket is now up for consideration. Thank you very much. -- |
|
@arc - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#131000 (status was 'resolved')
Searchable as RT131000$
The text was updated successfully, but these errors were encountered: