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

Owner: Nobody
Requestors: pmorch <peter [at] morch.com>
Cc:
AdminCc:

Operating System: Linux
PatchStatus: (no value)
Severity: Wishlist
Type: core
Perl Version: 5.20.2
Fixed In: (no value)

Attachments
0001-RT-131000-splice-doesn-t-honour-read-only-flag.patch
0001-v2-RT-131000-splice-doesn-t-honour-read-only-flag.patch



From: Peter Valdemar Mørch <peter [...] morch.com>
Subject: splice doesn't honor Internals::SvREADONLY.
To: perlbug [...] perl.org
Date: Wed, 15 Mar 2017 03:30:28 +0100
Download (untitled) / with headers
text/plain 15.7k

Message body is not shown because it is too large.

Download (untitled) / with headers
text/html 20.5k

Message body is not shown because it is too large.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 2.5k
On Wed, 15 Mar 2017 02:31:23 GMT, pmorch wrote: Show quoted text
> This is a bug report for perl from peter@morch.com, > generated with the help of perlbug 1.40 running under perl 5.20.2. > > > ----------------------------------------------------------------- > [Please describe your issue here] > > See: Calling splice() on Immutable Arrays > http://www.perlmonks.org/?node_id=1167665 > > or Bug #120130 for Const-Fast: splice doesn't honor > Internals::SvREADONLY > https://rt.cpan.org/Public/Bug/Display.html?id=120130 > > a tiny script to reproduce: > > #!/usr/bin/perl -w > use strict; > use feature qw(:5.10); > my @a = qw[a simple list]; > Internals::SvREADONLY( @a, 1 ); > # splice doesn't care about Internals::SvREADONLY and modifies @a > splice(@a, 1, 1, qw(not quite readonly)); > # "a not quite readonly list" > say join ' ', @a; > # This dies as expected with: Modification of a read-only value > attempted > unshift @a, qw*this is*; >
First, the obligatory disclaimers from lib/Internals.pod: ##### The Internals namespace is used by the core Perl development team to expose certain low level internals routines for testing and other purposes. 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 forward compatibility for some time. Nevertheless you can assume that any routine documented here is experimental or deprecated and you should find alternatives to their use. .... =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 version of perl used. 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. -- James E Keenan (jkeenan@cpan.org)
Date: Thu, 16 Mar 2017 12:47:52 +0000
CC: Perl5 Porters <perl5-porters [...] perl.org>
To: James E Keenan via RT <perlbug-followup [...] perl.org>
Subject: Re: [perl #131000] splice doesn't honor Internals::SvREADONLY.
From: Aaron Crane <arc [...] cpan.org>
Download (untitled) / with headers
text/plain 1.7k
James E Keenan via RT <perlbug-followup@perl.org> wrote: Show quoted text
> First, the obligatory disclaimers from lib/Internals.pod:
[snip] Show quoted text
> 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.
I think this is definitely a bug that should be fixed. Internals::SvREADONLY is merely the most obvious way for Perl code to make an otherwise-vanilla array read-only, but there are plenty of read-only arrays exposed to Perl space. For example, this should throw a "Modification of a read-only value attempted" exception: 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 onto a read-only array, I think this is deliberate. The attached patch makes all three builtins behave in the same way as push: read-only arrays yield an exception, unless the "modification" doesn't actually change anything. Sawyer, is this OK to merge before 5.26.0? It eliminates a "modification of a read-only value" exception for the case of unshifting an empty list onto a read-only array, but adds one (fixing a bug) for the case of modifying a read-only array using splice. I'll merge it if you say I should. -- Aaron Crane ** http://aaroncrane.co.uk/

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

To: Perl5 Porters <perl5-porters [...] perl.org>
Subject: Re: [perl #131000] splice doesn't honor Internals::SvREADONLY.
From: Zefram <zefram [...] fysh.org>
Date: Thu, 16 Mar 2017 13:14:21 +0000
Download (untitled) / with headers
text/plain 489b
Aaron Crane wrote: Show quoted text
>Given that pp_push contains specific code to permit push-of-nothing >onto a read-only array, I think this is deliberate.
It would be better to make this consistent in the other direction. push and unshift are write operations even if the attempted modification is null; better to consistently prohibit them on read-only arrays. Null modifications in other contexts (such as appending empty string to a string) are generally prohibited on read-only operands. -zefram
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 957b
On Thu, 16 Mar 2017 12:48:46 GMT, arc wrote: Show quoted text
> > I think this is definitely a bug that should be fixed. > Internals::SvREADONLY is merely the most obvious way for Perl code to > make an otherwise-vanilla array read-only, but there are plenty of > read-only arrays exposed to Perl space. For example, this should throw > a "Modification of a read-only value attempted" exception: > > > Sawyer, is this OK to merge before 5.26.0? It eliminates a > "modification of a read-only value" exception for the case of > unshifting an empty list onto a read-only array, but adds one (fixing > a bug) for the case of modifying a read-only array using splice. I'll > merge it if you say I should.
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. -- James E Keenan (jkeenan@cpan.org)
From: Leon Timmermans <fawaka [...] gmail.com>
Subject: Re: [perl #131000] splice doesn't honor Internals::SvREADONLY.
To: Aaron Crane <arc [...] cpan.org>
Date: Fri, 17 Mar 2017 11:41:38 +0100
CC: James E Keenan via RT <perlbug-followup [...] perl.org>, Perl5 Porters <perl5-porters [...] perl.org>
Download (untitled) / with headers
text/plain 2.1k
On Thu, Mar 16, 2017 at 1:47 PM, Aaron Crane <arc@cpan.org> wrote: Show quoted text
> James E Keenan via RT <perlbug-followup@perl.org> wrote:
>> First, the obligatory disclaimers from lib/Internals.pod:
> [snip]
>> 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.
> > I think this is definitely a bug that should be fixed. > Internals::SvREADONLY is merely the most obvious way for Perl code to > make an otherwise-vanilla array read-only, but there are plenty of > read-only arrays exposed to Perl space. For example, this should throw > a "Modification of a read-only value attempted" exception: > > splice @{^CAPTURE}, 0, 0, "oops"; > > just as this does: > > unshift @{^CAPTURE}, "oops";
Agreed. This is clearly a bug. Show quoted text
> 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 > onto a read-only array, I think this is deliberate. The attached patch > makes all three builtins behave in the same way as push: read-only > arrays yield an exception, unless the "modification" doesn't actually > change anything.
That is a surprising inconsistency. I don't think it's deliberate though, based on the git history. Show quoted text
> Sawyer, is this OK to merge before 5.26.0? It eliminates a > "modification of a read-only value" exception for the case of > unshifting an empty list onto a read-only array, but adds one (fixing > a bug) for the case of modifying a read-only array using splice. I'll > merge it if you say I should.
I can't think of any technical reason not to fix the splice thing right now. The fix is trivial and clearly solving a bug. I don't think the other part is urgent, and apparently it's more controversial, so I'd leave that for later. Leon
CC: James E Keenan via RT <perlbug-followup [...] perl.org>, Perl5 Porters <perl5-porters [...] perl.org>
Date: Fri, 17 Mar 2017 12:07:03 +0000
To: Leon Timmermans <fawaka [...] gmail.com>
Subject: Re: [perl #131000] splice doesn't honor Internals::SvREADONLY.
From: Aaron Crane <arc [...] cpan.org>
Download (untitled) / with headers
text/plain 837b
Leon Timmermans <fawaka@gmail.com> wrote: Show quoted text
> I can't think of any technical reason not to fix the splice thing > right now. The fix is trivial and clearly solving a bug. I don't think > the other part is urgent, and apparently it's more controversial, so > I'd leave that for later.
I've attached a replacement patch that fixes the bug in splice (without providing an exemption for non-modifying splices), and adds tests for the existing behaviour in push and unshift. The bug fix amounts to the addition of the following two lines of code in pp_splice: if (SvREADONLY(ary)) Perl_croak_no_modify(); It seems to me that we wouldn't be serving Perl's users well to require them to wait an extra year for a stable release of Perl that includes the trivial fix for this known bug. -- Aaron Crane ** http://aaroncrane.co.uk/

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

CC: Leon Timmermans <fawaka [...] gmail.com>, James E Keenan via RT <perlbug-followup [...] perl.org>, Perl5 Porters <perl5-porters [...] perl.org>
Date: Fri, 17 Mar 2017 15:09:48 +0000
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #131000] splice doesn't honor Internals::SvREADONLY.
To: Aaron Crane <arc [...] cpan.org>
Download (untitled) / with headers
text/plain 1.2k
On Fri, Mar 17, 2017 at 12:07:03PM +0000, Aaron Crane wrote: Show quoted text
> Leon Timmermans <fawaka@gmail.com> wrote:
> > I can't think of any technical reason not to fix the splice thing > > right now. The fix is trivial and clearly solving a bug. I don't think > > the other part is urgent, and apparently it's more controversial, so > > I'd leave that for later.
> > I've attached a replacement patch that fixes the bug in splice > (without providing an exemption for non-modifying splices), and adds > tests for the existing behaviour in push and unshift. > > The bug fix amounts to the addition of the following two lines of code > in pp_splice: > > if (SvREADONLY(ary)) > Perl_croak_no_modify(); > > It seems to me that we wouldn't be serving Perl's users well to > require them to wait an extra year for a stable release of Perl that > includes the trivial fix for this known bug.
Except that it could make code which currently works fail. It's probably naughty code, but we have a long history of delaying fixes to blead because it breaks naughty CPAN modules. I think it should wait for 5.27.x. -- Technology is dominated by two types of people: those who understand what they do not manage, and those who manage what they do not understand.
From: Sawyer X <xsawyerx [...] gmail.com>
Subject: Re: [perl #131000] splice doesn't honor Internals::SvREADONLY.
To: perl5-porters [...] perl.org
Date: Sat, 18 Mar 2017 18:37:23 +0100
Download (untitled) / with headers
text/plain 1.3k
On 03/17/2017 04:09 PM, Dave Mitchell wrote: Show quoted text
> On Fri, Mar 17, 2017 at 12:07:03PM +0000, Aaron Crane wrote:
>> Leon Timmermans <fawaka@gmail.com> wrote:
>>> I can't think of any technical reason not to fix the splice thing >>> right now. The fix is trivial and clearly solving a bug. I don't think >>> the other part is urgent, and apparently it's more controversial, so >>> I'd leave that for later.
>> I've attached a replacement patch that fixes the bug in splice >> (without providing an exemption for non-modifying splices), and adds >> tests for the existing behaviour in push and unshift. >> >> The bug fix amounts to the addition of the following two lines of code >> in pp_splice: >> >> if (SvREADONLY(ary)) >> Perl_croak_no_modify(); >> >> It seems to me that we wouldn't be serving Perl's users well to >> require them to wait an extra year for a stable release of Perl that >> includes the trivial fix for this known bug.
> Except that it could make code which currently works fail. It's probably > naughty code, but we have a long history of delaying fixes to blead > because it breaks naughty CPAN modules. > > I think it should wait for 5.27.x.
I had expressed the same opinion on IRC. (I'm still open to continuing discussing this, though, but on Monday a new release is out - with or without this.)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.5k
On Sat, 18 Mar 2017 17:38:07 GMT, xsawyerx@gmail.com wrote: Show quoted text
> > > On 03/17/2017 04:09 PM, Dave Mitchell wrote:
> > On Fri, Mar 17, 2017 at 12:07:03PM +0000, Aaron Crane wrote:
> >> Leon Timmermans <fawaka@gmail.com> wrote:
> >>> I can't think of any technical reason not to fix the splice thing > >>> right now. The fix is trivial and clearly solving a bug. I don't think > >>> the other part is urgent, and apparently it's more controversial, so > >>> I'd leave that for later.
> >> I've attached a replacement patch that fixes the bug in splice > >> (without providing an exemption for non-modifying splices), and adds > >> tests for the existing behaviour in push and unshift. > >> > >> The bug fix amounts to the addition of the following two lines of code > >> in pp_splice: > >> > >> if (SvREADONLY(ary)) > >> Perl_croak_no_modify(); > >> > >> It seems to me that we wouldn't be serving Perl's users well to > >> require them to wait an extra year for a stable release of Perl that > >> includes the trivial fix for this known bug.
> > Except that it could make code which currently works fail. It's probably > > naughty code, but we have a long history of delaying fixes to blead > > because it breaks naughty CPAN modules. > > > > I think it should wait for 5.27.x.
> > I had expressed the same opinion on IRC. > > (I'm still open to continuing discussing this, though, but on Monday a > new release is out - with or without this.) >
With perl-5.26.0 having been released, this ticket is now up for consideration. Thank you very much. -- James E Keenan (jkeenan@cpan.org)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 235b
Fixed in 3275d25a1e4129bdf23c447f60be4348af4dfe19. I have retained the perhaps-surprising oddity that C<< push @readonly, () >> throws no exception, without extending that empty-list special case to unshift or splice. -- Aaron Crane


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