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

splice doesn't honor Internals::SvREADONLY. #15923

Closed
p5pRT opened this issue Mar 15, 2017 · 15 comments
Closed

splice doesn't honor Internals::SvREADONLY. #15923

p5pRT opened this issue Mar 15, 2017 · 15 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Mar 15, 2017

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

Searchable as RT131000$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 15, 2017

From peter@morch.com

Created by peter@morch.com

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

Perl Info

Flags:
    category=core
    severity=low

Site configuration information for perl 5.20.2:

Configured by Debian Project at Fri Jul 22 15:47:27 UTC 2016.

Summary of my perl5 (revision 5 version 20 subversion 2) configuration:

  Platform:
    osname=linux, osvers=3.16.0-4-amd64,
archname=x86_64-linux-gnu-thread-multi
    uname='linux himalia 3.16.0-4-amd64 #1 smp debian 3.16.7-ckt25-2+deb8u3
(2016-07-02) x86_64 gnulinux '
    config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN
-D_FORTIFY_SOURCE=2 -g -O2 -fstack-protector-strong -Wformat
-Werror=format-security -Dldflags= -Wl,-z,relro -Dlddlflags=-shared
-Wl,-z,relro -Dcccdlflags=-fPIC -Darchname=x86_64-linux-gnu -Dprefix=/usr
-Dprivlib=/usr/share/perl/5.20
-Darchlib=/usr/lib/x86_64-linux-gnu/perl/5.20 -Dvendorprefix=/usr
-Dvendorlib=/usr/share/perl5
-Dvendorarch=/usr/lib/x86_64-linux-gnu/perl5/5.20 -Dsiteprefix=/usr/local
-Dsitelib=/usr/local/share/perl/5.20.2
-Dsitearch=/usr/local/lib/x86_64-linux-gnu/perl/5.20.2
-Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3
-Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3
-Dusesitecustomize -Duse64bitint -Dman1ext=1 -Dman3ext=3perl
-Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Ud_ualarm -Uusesfio -Uusenm
-Ui_libutil -Uversiononly -DDEBUGGING=-g -Doptimize=-O2 -Duseshrplib
-Dlibperl=libperl.so.5.20.2 -des'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv
-fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64',
    optimize='-O2 -g',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv
-fno-strict-aliasing -pipe -I/usr/local/include'
    ccversion='', gccversion='4.9.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 /usr/lib/gcc/x86_64-linux-gnu/4.9/include-fixed
/usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib
/usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
    libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
    perllibs=-ldl -lm -lpthread -lc -lcrypt
    libc=libc-2.19.so, so=so, useshrplib=true, libperl=libperl.so.5.20
    gnulibc_version='2.19'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -L/usr/local/lib
-fstack-protector'

Locally applied patches:
    DEBPKG:debian/cpan_definstalldirs - Provide a sensible INSTALLDIRS
default for modules installed from CPAN.
    DEBPKG:debian/db_file_ver - http://bugs.debian.org/340047 Remove overly
restrictive DB_File version check.
    DEBPKG:debian/doc_info - Replace generic man(1) instructions with
Debian-specific information.
    DEBPKG:debian/enc2xs_inc - http://bugs.debian.org/290336 Tweak enc2xs
to follow symlinks and ignore missing @INC directories.
    DEBPKG:debian/errno_ver - http://bugs.debian.org/343351 Remove Errno
version check due to upgrade problems with long-running processes.
    DEBPKG:debian/libperl_embed_doc - http://bugs.debian.org/186778 Note
that libperl-dev package is required for embedded linking
    DEBPKG:fixes/respect_umask - Respect umask during installation
    DEBPKG:debian/writable_site_dirs - Set umask approproately for site
install directories
    DEBPKG:debian/extutils_set_libperl_path - EU:MM: set location of
libperl.a under /usr/lib
    DEBPKG:debian/no_packlist_perllocal - Don't install .packlist or
perllocal.pod for perl or vendor
    DEBPKG:debian/prefix_changes - Fiddle with *PREFIX and variables
written to the makefile
    DEBPKG:debian/fakeroot - Postpone LD_LIBRARY_PATH evaluation to the
binary targets.
    DEBPKG:debian/instmodsh_doc - Debian policy doesn't install .packlist
files for core or vendor.
    DEBPKG:debian/ld_run_path - Remove standard libs from LD_RUN_PATH as
per Debian policy.
    DEBPKG:debian/libnet_config_path - Set location of libnet.cfg to
/etc/perl/Net as /usr may not be writable.
    DEBPKG:debian/mod_paths - Tweak @INC ordering for Debian
    DEBPKG:debian/module_build_man_extensions -
http://bugs.debian.org/479460 Adjust Module::Build manual page extensions
for the Debian Perl policy
    DEBPKG:debian/prune_libs - http://bugs.debian.org/128355 Prune the list
of libraries wanted to what we actually need.
    DEBPKG:fixes/net_smtp_docs - [rt.cpan.org #36038]
http://bugs.debian.org/100195 Document the Net::SMTP 'Port' option
    DEBPKG:debian/perlivp - http://bugs.debian.org/510895 Make perlivp skip
include directories in /usr/local
    DEBPKG:debian/deprecate-with-apt - http://bugs.debian.org/747628 Point
users to Debian packages of deprecated core modules
    DEBPKG:debian/squelch-locale-warnings - http://bugs.debian.org/508764
Squelch locale warnings in Debian package maintainer scripts
    DEBPKG:debian/skip-upstream-git-tests - Skip tests specific to the
upstream Git repository
    DEBPKG:debian/patchlevel - http://bugs.debian.org/567489 List packaged
patches for 5.20.2-3+deb8u6 in patchlevel.h
    DEBPKG:debian/skip-kfreebsd-crash - http://bugs.debian.org/628493 [perl
#96272] Skip a crashing test case in t/op/threads.t on GNU/kFreeBSD
    DEBPKG:fixes/document_makemaker_ccflags - http://bugs.debian.org/628522
[rt.cpan.org #68613] Document that CCFLAGS should include $Config{ccflags}
    DEBPKG:debian/find_html2text - http://bugs.debian.org/640479 Configure
CPAN::Distribution with correct name of html2text
    DEBPKG:debian/perl5db-x-terminal-emulator.patch -
http://bugs.debian.org/668490 Invoke x-terminal-emulator rather than xterm
in perl5db.pl
    DEBPKG:debian/cpan-missing-site-dirs - http://bugs.debian.org/688842
Fix CPAN::FirstTime defaults with nonexisting site dirs if a parent is
writable
    DEBPKG:fixes/memoize_storable_nstore - [rt.cpan.org #77790]
http://bugs.debian.org/587650 Memoize::Storable: respect 'nstore' option
not respected
    DEBPKG:debian/regen-skip - Skip a regeneration check in unrelated git
repositories
    DEBPKG:fixes/regcomp-mips-optim - [perl #122817]
http://bugs.debian.org/754054 Downgrade the optimization of regcomp.c on
mips and mipsel due to a gcc-4.9 bug
    DEBPKG:debian/makemaker-pasthru - http://bugs.debian.org/758471 Pass LD
settings through to subdirectories
    DEBPKG:fixes/perldoc-less-R - [rt.cpan.org #98636]
http://bugs.debian.org/758689 Tell the 'less' pager to allow terminal
escape sequences
    DEBPKG:fixes/pod_man_reproducible_date - http://bugs.debian.org/759405
Support POD_MAN_DATE in Pod::Man for the left-hand footer
    DEBPKG:fixes/io_uncompress_gunzip_inmemory -
http://bugs.debian.org/747363 [rt.cpan.org #95494] Fix gunzip to in-memory
file handle
    DEBPKG:fixes/socket_test_recv_fix - http://bugs.debian.org/758718 [perl
#122657] Compare recv return value to peername in socket test
    DEBPKG:fixes/hurd_socket_recv_todo - http://bugs.debian.org/758718
[perl #122657] TODO checking the result of recv() on hurd
    DEBPKG:fixes/regexp-performance - [0fa70a0]
http://bugs.debian.org/777556 [perl #123743] simpify and speed up /.*.../
handling
    DEBPKG:fixes/failed_require_diagnostics - http://bugs.debian.org/781120
[perl #123270] Report inaccesible file on failed require
    DEBPKG:fixes/array-cloning - http://bugs.debian.org/779357 [perl
#124127] [902d169] fix cloning arrays with unused elements
    DEBPKG:fixes/perldb-threads - http://bugs.debian.org/779357 [perl
#124127] [41ef2c6] lib/perl5db.pl: Restore noop lock prototype
    DEBPKG:fixes/CVE-2015-8607_file_spec_taint_fix - ensure
File::Spec::canonpath() preserves taint
    DEBPKG:fixes/encode-unicode-bom - http://bugs.debian.org/798727 [
rt.cpan.org #107043] Address
https://rt.cpan.org/Public/Bug/Display.html?id=107043
    DEBPKG:debian/encode-unicode-bom-doc - http://bugs.debian.org/798727
Document Debian backport of Encode::Unicode fix
    DEBPKG:debian/kfreebsd-softupdates - http://bugs.debian.org/796798 Work
around Debian Bug#796798
    DEBPKG:fixes/CVE-2016-2381_duplicate_env - remove duplicate environment
variables from environ
    DEBPKG:debian/debugperl-compat-fix - [perl #127212]
http://bugs.debian.org/810326 Disable PERL_TRACK_MEMPOOL for debugging
builds
    DEBPKG:fixes/CVE-2015-8853_regexp_hang - http://bugs.debian.org/821848
[perl #123562] PATCH [perl #123562] Regexp-matching "hangs"
    DEBPKG:fixes/utf8_regexp_crash - http://bugs.debian.org/820328 [perl
#124109] save_re_context(): do "local $n" with no PL_curpm
    DEBPKG:fixes/regcomp_whitespace_fix - http://bugs.debian.org/820328
[perl #124109] Perl_save_re_context(): re-indent after last commit
    DEBPKG:fixes/5.20.3/eval_label_crash - http://bugs.debian.org/822336
[perl #123652] eval {label:} crash
    DEBPKG:fixes/5.20.3/preserve_record_separator -
http://bugs.debian.org/822336 [perl #123218] "preserve" $/ if set to a bad
value
    DEBPKG:fixes/5.20.3/test_count_base_rs - http://bugs.debian.org/822336
Fix test count in t/base/rs.t
    DEBPKG:fixes/5.20.3/remove_get_magic - http://bugs.debian.org/822336
[perl #123739] Remove get-magic from $/
    DEBPKG:fixes/5.20.3/speed_up_scalar_g - http://bugs.debian.org/822336
[perl #123202] speed up scalar //g against tainted strings
    DEBPKG:fixes/5.20.3/accidental_all_features -
http://bugs.debian.org/822336 Stop $^H |= 0x1c020000 from enabling all
features
    DEBPKG:fixes/5.20.3/multidimensional_arrays_utf8 -
http://bugs.debian.org/822336 [perl #124113] Make check for
multi-dimensional arrays be UTF8-aware
    DEBPKG:fixes/5.20.3/unquoted_utf8_heredoc_terminators -
http://bugs.debian.org/822336 Allow unquoted UTF-8 HERE-document terminators
    DEBPKG:fixes/5.20.3/parentheses_ambiguous_warning_utf8_functions -
http://bugs.debian.org/822336 Fix "...without parentheses is ambuguous"
warning for UTF-8 function names
    DEBPKG:fixes/5.20.3/leak_namepv_copy - http://bugs.debian.org/822336
[perl #123786] don't leak the temp utf8 copy of namepv
    DEBPKG:fixes/5.20.3/h2ph_hex_constants - http://bugs.debian.org/822336
h2ph: correct handling of hex constants for the preamble
    DEBPKG:fixes/5.20.3/leftbracket_XTERMORDORDOR -
http://bugs.debian.org/822336 [perl #123711] Fix crash with 0-5x-l{0}
    DEBPKG:fixes/5.20.3/fatalize_warnings_unwinding -
http://bugs.debian.org/822336 [perl #123398] don't fatalize warnings during
unwinding (#123398)
    DEBPKG:fixes/5.20.3/setpgrp - http://bugs.debian.org/822336
=?UTF-8?q?Don=E2=80=99t=20treat=20setpgrp($nonzero)=20as=20setpgr?=
=?UTF-8?q?p(1)?=
    DEBPKG:fixes/5.20.3/death_unwinding_crash -
http://bugs.debian.org/822336 [perl #124156] RT #124156: death during
unwinding causes crash
    DEBPKG:fixes/5.20.3/stashpvn_crash - http://bugs.debian.org/822336
[perl #125541] Fix crash with %::=(); J->${\"::"}
    DEBPKG:fixes/5.20.3/possessive_quantifier -
http://bugs.debian.org/822336 [perl #125825] PATCH: [perl 125825] {n}+
possessive quantifier broken
    DEBPKG:fixes/5.20.3/quoted_code_crash - http://bugs.debian.org/822336
[perl #123712] Fix /$a[/ parsing
    DEBPKG:fixes/5.20.3/checking_sub_inwhat - http://bugs.debian.org/822336
[perl #123712] Don't check sub_inwhat
    DEBPKG:fixes/5.20.3/yylex_loop - http://bugs.debian.org/822336 Fix hang
with "@{"
    DEBPKG:fixes/5.20.3/docs/op - http://bugs.debian.org/822336 Fix apidocs
for OP_TYPE_IS(_OR_WAS) - arguments separated by |, not ,.
    DEBPKG:fixes/5.20.3/docs/encoding - http://bugs.debian.org/822336
perlpodspec: Corrections/adds to detecting =encoding
    DEBPKG:fixes/5.20.3/docs/SvPV_set - http://bugs.debian.org/822336
improve SvPV_set's docs, it really shouldn't be public API
    DEBPKG:fixes/5.20.3/docs/autodie - http://bugs.debian.org/822336 Fix
warning message regarding "use autodie" and "use open".
    DEBPKG:fixes/5.20.3/docs/autodie_2_26 - http://bugs.debian.org/822336
perlunicook: Note that autodie >= 2.26 should be okay with "use open".
    DEBPKG:fixes/5.20.3/docs/setenv - http://bugs.debian.org/822336 Fix
setenv() replacement documentation in perlclib
    DEBPKG:fixes/5.20.3/docs/clib_caution - http://bugs.debian.org/822336
perlhacktips: Add caution about clib ptr returns to static memory
    DEBPKG:fixes/5.20.3/docs/perlunicook_typos -
http://bugs.debian.org/822336 Fix minor code typos in perlunicook
    DEBPKG:fixes/5.20.3/docs/ook_example - http://bugs.debian.org/822336
[perl #122322] Update OOK example in perlguts
    DEBPKG:fixes/5.20.3/docs/study_noop - http://bugs.debian.org/822336
perlfunc: mention that study() is currently a noop
    DEBPKG:fixes/CVE-2016-1238/remove-dot-when-loading - [perl #127834]
(perl #127834) remove . from the end of @INC if complex modules are loaded
    DEBPKG:fixes/CVE-2016-1238/remove-dot-in-padwalker - [perl #127834]
perl5db.pl: ensure PadWalker is loaded from standard paths
    DEBPKG:fixes/CVE-2016-1238/remove-dot-in-dist - [perl #127834] dist/:
remove . from @INC when loading optional modules
    DEBPKG:fixes/CVE-2016-1238/remove-dot-in-cpan - [perl #127834] cpan/:
remove . from @INC when loading optional modules
    DEBPKG:fixes/CVE-2016-1238/customized-encode - Update customized.dat
for cpan/Encode/Encode.pm
    DEBPKG:debian/CVE-2016-1238/test-suite-without-dot - [perl #127810]
Patch unit tests to explicitly insert "." into @INC when needed.
    DEBPKG:debian/CVE-2016-1238/eumm-without-dot - [perl #127810] Add
PERL_USE_UNSAFE_INC support to EU::MM for fortify_inc support.
    DEBPKG:debian/CVE-2016-1238/cpan-without-dot - [perl #127810] Set
PERL_USE_UNSAFE_INC for cpan usage
    DEBPKG:debian/CVE-2016-1238/mb-without-dot - Make Module::Build set
PERL_USE_UNSAFE_INC
    DEBPKG:debian/CVE-2016-1238/sitecustomize-in-etc - Look for
sitecustomize.pl in /etc/perl rather than sitelib on Debian systems
    DEBPKG:fixes/xsloader-eval - [rt.cpan.org #115808]
http://bugs.debian.org/829578
=?UTF-8?q?Don=E2=80=99t=20let=20XSLoader=20load=20relative=20path?=
=?UTF-8?q?s?=


@INC for perl 5.20.2:
    /etc/perl
    /usr/local/lib/x86_64-linux-gnu/perl/5.20.2
    /usr/local/share/perl/5.20.2
    /usr/lib/x86_64-linux-gnu/perl5/5.20
    /usr/share/perl5
    /usr/lib/x86_64-linux-gnu/perl/5.20
    /usr/share/perl/5.20
    /usr/local/lib/site_perl


Environment for perl 5.20.2:
    HOME=/home/peter
    LANG=en_US.UTF-8
    LANGUAGE=en_DK:en
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)

PATH=.:/home/peter/bin/local/sshuttleCapmon:/home/peter/bin/local/ngrok:/home/peter/bin/local/entr:/home/peter/bin/local/capmon-node:/home/peter/bin/local/capmon:/home/peter/bin:/home/peter/bin/modules/monitorfiles:/home/peter/bin/modules/mgrep:/usr/local/bin:/usr/bin:/bin:/usr/games:/sbin:/usr/sbin:/usr/bin/X11
    PERL_BADLANG (unset)
    SHELL=/bin/bash


-- 
Peter Valdemar Mørch
http://www.morch.com

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 16, 2017

From @jkeenan

On Wed, 15 Mar 2017 02​:31​:23 GMT, pmorch wrote​:

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)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 16, 2017

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 16, 2017

From @arc

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

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/

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 16, 2017

From @arc

0001-RT-131000-splice-doesn-t-honour-read-only-flag.patch
From 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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 16, 2017

From zefram@fysh.org

Aaron Crane wrote​:

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 16, 2017

From @jkeenan

On Thu, 16 Mar 2017 12​:48​:46 GMT, arc wrote​:

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)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 17, 2017

From @Leont

On Thu, Mar 16, 2017 at 1​:47 PM, Aaron Crane <arc@​cpan.org> wrote​:

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.

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.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 17, 2017

From @arc

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.

--
Aaron Crane ** http​://aaroncrane.co.uk/

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 17, 2017

From @arc

0001-v2-RT-131000-splice-doesn-t-honour-read-only-flag.patch
From 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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 17, 2017

From @iabyn

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.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 18, 2017

From @xsawyerx

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 1, 2017

From @jkeenan

On Sat, 18 Mar 2017 17​:38​:07 GMT, xsawyerx@​gmail.com wrote​:

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)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 15, 2017

From @arc

Fixed in 3275d25. 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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 15, 2017

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

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.