'x' operator on list causes segfault with possible stack corruption

p5pRT commented 9 years ago

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

Searchable as RT125937$

p5pRT commented 9 years ago

From @dcollinsn

Greetings Porters\,

I have compiled bleadperl with the afl-gcc compiler using:

./Configure -Dusedevel -Dprefix='/usr/local/perl-afl' -Dcc=afl-gcc -Duselongdouble -Duse64bitint -Doptimize=-g -Uversiononly -Uman1dir -Uman3dir -des AFL_HARDEN=1 make

And then fuzzed the resulting binary using:

AFL_NO_VAR_CHECK=1 afl-fuzz -i in -o out bin/perl @@

After reducing testcases using `afl-tmin` and filtering out testcases that are merely iterations of "#!perl -u"\, I have located the following testcase that triggers a segmentation fault in the perl interpreter. The testcase is the file:

f$0(d..M)x{}

NB: f$0(a..n)x{} (a 14-item list) also fails with this behavior\, a..m (a 13-item list) does not. However\, with ranges ("a..n") very close to the minimum\, the program occasionally fails with an out-of-memory error instead. In order to consistently segfault\, I am using ("a..z") in the gdb and valgrind runs.

In old versions of Perl\, this caused an out of memory error. This now causes a segfault in perl and miniperl. The git bisect appears to point to a commit that is relevant to the testcase.

**GDB**

Localizes the segfault to memcpy.S but is not able to provide a backtrace of any kind\, presumably because the stack has already been clobbered by the time that GDB is able to step in.

**VALGRIND**

dcollins@nagios:\~$ valgrind /usr/local/perl-afl/bin/perl crash ==22077== Memcheck\, a memory error detector ==22077== Copyright (C) 2002-2010\, and GNU GPL'd\, by Julian Seward et al. ==22077== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info ==22077== Command: /usr/local/perl-afl/bin/perl crash ==22077== Out of memory during array extend at crash line 1. ==22077== ==22077== HEAP SUMMARY: ==22077== in use at exit: 83\,233 bytes in 497 blocks ==22077== total heap usage: 703 allocs\, 206 frees\, 115\,135 bytes allocated ==22077== ==22077== LEAK SUMMARY: ==22077== definitely lost: 0 bytes in 0 blocks ==22077== indirectly lost: 0 bytes in 0 blocks ==22077== possibly lost: 11\,791 bytes in 266 blocks ==22077== still reachable: 71\,442 bytes in 231 blocks ==22077== suppressed: 0 bytes in 0 blocks ==22077== Rerun with --leak-check=full to see details of leaked memory ==22077== ==22077== For counts of detected and suppressed errors\, rerun with: -v ==22077== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 25 from 8)

**BISECT**

b3b27d017bd3d61f6cbc3eaf8465b8d98d86a024 is the first bad commit commit b3b27d017bd3d61f6cbc3eaf8465b8d98d86a024 Author: David Mitchell \davem@iabyn\.com Date: Wed Mar 11 12:25:58 2015 +0000

repeat op: avoid integer overflows

For the list variant of the x operator\, the overflow detection code doesn't always work\, resulting in the stack not being extended enough.

There are two places where an overflow could occur: calculating how many stack items to extend by (items * count)\, and passing to repeatcpy() how many bytes to copy 'items' SV pointers (items * sizeof(const SV *)).

The overflow detection was generally a mess; checking for overflow using a value (max) that may have already overflown; checking whether 'max' had overflown incorrectly\, and not checking (items * sizeof(const SV *) at all (perhaps hoping that the other checks would be a superset of this).

Also\, some of the vars were still I32s; promote to 64-bit capable types.

Finally\, the signature of Perl_repeatcpy() still has an I32 len; that really should be changed to IV at some point; but for now I've just ensured that the callers (list 'x' and scalar 'x') don't wrap. I haven't put in a check for the only other core caller of repeatcpy()\, S_study_chunk()\, since that would only become an issue compiling a pattern with a fixed or floating substr within it of length > 2**31.

:100644 100644 f7957256e80945e4fec2f0fb1947c492c4c19cc2 ec0f9d31ec203d942bd8570c97f7f06f6294f403 M pp.c bisect run success

**PERL -V**

dcollins@nagios:\~$ /usr/local/perl-afl/bin/perl -V Summary of my perl5 (revision 5 version 23 subversion 3) configuration: Commit id: 679f623f366bc81022b7c77de4bc4302828303c6 Platform: osname=linux\, osvers=2.6.32-5-686\, archname=i686-linux-64int-ld uname='linux nagios 2.6.32-5-686 #1 smp tue may 13 16:33:32 utc 2014 i686 gnulinux ' config_args='-Dusedevel -Dprefix=/usr/local/perl-afl -Dcc=afl-gcc -Duselongdouble -Duse64bitint -Doptimize=-g -Uversiononly -Uman1dir -Uman3dir -des' hint=recommended\, useposix=true\, d_sigaction=define useithreads=undef\, usemultiplicity=undef use64bitint=define\, use64bitall=undef\, uselongdouble=define usemymalloc=n\, bincompat5005=undef Compiler: cc='afl-gcc'\, ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'\, optimize='-g'\, cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion=''\, gccversion='4.4.5'\, gccosandvers='' intsize=4\, longsize=4\, ptrsize=4\, doublesize=8\, byteorder=12345678\, doublekind=3 d_longlong=define\, longlongsize=8\, d_longdbl=define\, longdblsize=12\, longdblkind=3 ivtype='long long'\, ivsize=8\, nvtype='long double'\, nvsize=12\, Off_t='off_t'\, lseeksize=8 alignbytes=4\, prototype=define Linker and Libraries: ld='afl-gcc'\, ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /usr/lib/gcc/i486-linux-gnu/4.4.5/include-fixed /usr/lib /lib/../lib /usr/lib/../lib /lib /usr/lib/i486-linux-gnu /usr/lib64 libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.11.3.so\, so=so\, useshrplib=false\, libperl=libperl.a gnulibc_version='2.11.3' Dynamic Linking: dlsrc=dl_dlopen.xs\, dlext=so\, d_dlsymun=undef\, ccdlflags='-Wl\,-E' cccdlflags='-fPIC'\, lddlflags='-shared -g -L/usr/local/lib -fstack-protector'

Characteristics of this binary (from libperl): Compile-time options: HAS_TIMES PERLIO_LAYERS PERL_COPY_ON_WRITE PERL_DONT_CREATE_GVSV PERL_HASH_FUNC_ONE_AT_A_TIME_HARD PERL_MALLOC_WRAP PERL_PRESERVE_IVUV PERL_USE_DEVEL USE_64_BIT_INT USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_LOCALE_TIME USE_LONG_DOUBLE USE_PERLIO USE_PERL_ATOF Built under linux Compiled at Aug 26 2015 15:51:11 @INC: /usr/local/perl-afl/lib/site_perl/5.23.3/i686-linux-64int-ld /usr/local/perl-afl/lib/site_perl/5.23.3 /usr/local/perl-afl/lib/5.23.3/i686-linux-64int-ld /usr/local/perl-afl/lib/5.23.3 /usr/local/perl-afl/lib/site_perl/5.23.2 /usr/local/perl-afl/lib/site_perl .

p5pRT commented 9 years ago

From @iabyn

On Fri\, Aug 28\, 2015 at 09:12:12PM -0700\, Dan Collins wrote:

f$0(d..M)x{}

NB: f$0(a..n)x{} (a 14-item list) also fails with this behavior\, a..m (a 13-item list) does not. However\, with ranges ("a..n") very close to the minimum\, the program occasionally fails with an out-of-memory error instead. In order to consistently segfault\, I am using ("a..z") in the gdb and valgrind runs.

In old versions of Perl\, this caused an out of memory error. This now causes a segfault in perl and miniperl. The git bisect appears to point to a commit that is relevant to the testcase.

Unfortunately I can't reproduce this. It's made harder by the fact that the anon hash {} is being used in a numeric context as the repeat count\, and a ref in numeric context returns the address of the referent\, e.g.

$ perl -le'print 0+{}' 25509048

This makes the count highly dependent on OS etc. Also\, valgrind tends to make things have much higher addresses:

valgrind perl -le'print 0+{}' ... 79949192

so running it under valgrind can change its behaviour completely.

When I try running it with a small enough literal count to fit in memory\, I see the program successfully (if slowly) run to completion\, before rightfully complaining that it can't find the $0->f() method. For larger counts\, it fails instantly with "Out of memory!". For example:

# set virtual memory limit to 5Gb $ ulimit -v `perl -le 'print 5 * 1024 * 1024'`

$ p -e'f$0(d..M)x28_459_935' Can't locate object method "f" via package "-e" (perhaps you forgot to load "-e"?) at -e line 1.

$ p -e'f$0(d..M)x28_459_936' Out of memory! $

I don't see any different transitional behaviour near the ulimit (specifically no crashes). I also see the same behaviour with ulimits of 1Gb\, 2Gb and 4Gb and with no ulimit (although I didn't try many counts with the last\, as it tended to send my laptop into swap hell).

Are you able to reproduce it using a fixed ulimit and a literal count? And if so\, does valgrind give anything useful now?

Also\, what is your version of glibc?

Thanks.

-- Music lesson: a symbiotic relationship whereby a pupil's embellishments concerning the amount of practice performed since the last lesson are rewarded with embellishments from the teacher concerning the pupil's progress over the corresponding period.

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

From @dcollinsn

I can reproduce this:

dcollins@nagios:\~/perl$ ./perl -e 'f$0(d..M)x{}' Segmentation fault dcollins@nagios:\~/perl$ ./perl -e 'print 0+{}\, "\n"' 170597024 dcollins@nagios:\~/perl$ ./perl -e 'f$0(d..M)x170597024' Segmentation fault dcollins@nagios:\~/perl$ bash dcollins@nagios:\~/perl$ ulimit -v `perl -le 'print 1024*1024'` dcollins@nagios:\~/perl$ ./perl -e 'f$0(d..M)x170597024' Segmentation fault dcollins@nagios:\~/perl$ exit

My glibc is 2.11-1 from the debian repository.

GDB with the literal argument is equally useless\, however calling valgrind with the literal argument does force the segfaulting behavior instead of the OOM error:

dcollins@nagios:\~/perl$ valgrind ./perl -e 'f$0(d..M)x170597024' ==15004== Memcheck\, a memory error detector ==15004== Copyright (C) 2002-2010\, and GNU GPL'd\, by Julian Seward et al. ==15004== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info ==15004== Command: ./perl -e f$0(d..M)x170597024 ==15004== ==15004== Invalid write of size 1 ==15004== at 0x402591F: memcpy (mc_replace_strmem.c:497) ==15004== by 0x82ECD24: Perl_repeatcpy (string3.h:52) ==15004== by 0x8477653: Perl_pp_repeat (pp.c:1744) ==15004== by 0x836EABF: Perl_runops_standard (run.c:41) ==15004== by 0x810ABD2: perl_run (perl.c:2448) ==15004== by 0x8062C44: main (perlmain.c:116) ==15004== Address 0x4217a17 is 1 bytes before a block of size 8\,112 alloc'd ==15004== at 0x4023F50: malloc (vg_replace_malloc.c:236) ==15004== by 0x82D4537: Perl_safesysmalloc (util.c:149) ==15004== by 0x849BCE0: Perl_new_stackinfo (scope.c:64) ==15004== by 0x80F6E8C: Perl_init_stacks (perl.c:4055) ==15004== by 0x80FABB0: perl_construct (perl.c:249) ==15004== by 0x8062C1F: main (perlmain.c:110) ==15004== ==15004== Invalid write of size 1 ==15004== at 0x4025927: memcpy (mc_replace_strmem.c:497) ==15004== by 0x82ECD24: Perl_repeatcpy (string3.h:52) ==15004== by 0x8477653: Perl_pp_repeat (pp.c:1744) ==15004== by 0x836EABF: Perl_runops_standard (run.c:41) ==15004== by 0x810ABD2: perl_run (perl.c:2448) ==15004== by 0x8062C44: main (perlmain.c:116) ==15004== Address 0x4217a16 is 2 bytes before a block of size 8\,112 alloc'd ==15004== at 0x4023F50: malloc (vg_replace_malloc.c:236) ==15004== by 0x82D4537: Perl_safesysmalloc (util.c:149) ==15004== by 0x849BCE0: Perl_new_stackinfo (scope.c:64) ==15004== by 0x80F6E8C: Perl_init_stacks (perl.c:4055) ==15004== by 0x80FABB0: perl_construct (perl.c:249) ==15004== by 0x8062C1F: main (perlmain.c:110) ==15004== ==15004== Invalid write of size 1 ==15004== at 0x4025930: memcpy (mc_replace_strmem.c:497) ==15004== by 0x82ECD24: Perl_repeatcpy (string3.h:52) ==15004== by 0x8477653: Perl_pp_repeat (pp.c:1744) ==15004== by 0x836EABF: Perl_runops_standard (run.c:41) ==15004== by 0x810ABD2: perl_run (perl.c:2448) ==15004== by 0x8062C44: main (perlmain.c:116) ==15004== Address 0x4217a15 is 3 bytes before a block of size 8\,112 alloc'd ==15004== at 0x4023F50: malloc (vg_replace_malloc.c:236) ==15004== by 0x82D4537: Perl_safesysmalloc (util.c:149) ==15004== by 0x849BCE0: Perl_new_stackinfo (scope.c:64) ==15004== by 0x80F6E8C: Perl_init_stacks (perl.c:4055) ==15004== by 0x80FABB0: perl_construct (perl.c:249) ==15004== by 0x8062C1F: main (perlmain.c:110) ==15004== ==15004== Invalid write of size 1 ==15004== at 0x4025939: memcpy (mc_replace_strmem.c:497) ==15004== by 0x82ECD24: Perl_repeatcpy (string3.h:52) ==15004== by 0x8477653: Perl_pp_repeat (pp.c:1744) ==15004== by 0x836EABF: Perl_runops_standard (run.c:41) ==15004== by 0x810ABD2: perl_run (perl.c:2448) ==15004== by 0x8062C44: main (perlmain.c:116) ==15004== Address 0x4217a14 is 4 bytes before a block of size 8\,112 alloc'd ==15004== at 0x4023F50: malloc (vg_replace_malloc.c:236) ==15004== by 0x82D4537: Perl_safesysmalloc (util.c:149) ==15004== by 0x849BCE0: Perl_new_stackinfo (scope.c:64) ==15004== by 0x80F6E8C: Perl_init_stacks (perl.c:4055) ==15004== by 0x80FABB0: perl_construct (perl.c:249) ==15004== by 0x8062C1F: main (perlmain.c:110) ==15004== ==15004== Invalid read of size 1 ==15004== at 0x4025918: memcpy (mc_replace_strmem.c:497) ==15004== by 0x82ECD24: Perl_repeatcpy (string3.h:52) ==15004== by 0x8477653: Perl_pp_repeat (pp.c:1744) ==15004== by 0x836EABF: Perl_runops_standard (run.c:41) ==15004== by 0x810ABD2: perl_run (perl.c:2448) ==15004== by 0x8062C44: main (perlmain.c:116) ==15004== Address 0x4217a17 is 1 bytes before a block of size 8\,112 alloc'd ==15004== at 0x4023F50: malloc (vg_replace_malloc.c:236) ==15004== by 0x82D4537: Perl_safesysmalloc (util.c:149) ==15004== by 0x849BCE0: Perl_new_stackinfo (scope.c:64) ==15004== by 0x80F6E8C: Perl_init_stacks (perl.c:4055) ==15004== by 0x80FABB0: perl_construct (perl.c:249) ==15004== by 0x8062C1F: main (perlmain.c:110) ==15004== ==15004== Invalid read of size 1 ==15004== at 0x4025922: memcpy (mc_replace_strmem.c:497) ==15004== by 0x82ECD24: Perl_repeatcpy (string3.h:52) ==15004== by 0x8477653: Perl_pp_repeat (pp.c:1744) ==15004== by 0x836EABF: Perl_runops_standard (run.c:41) ==15004== by 0x810ABD2: perl_run (perl.c:2448) ==15004== by 0x8062C44: main (perlmain.c:116) ==15004== Address 0x4217a16 is 2 bytes before a block of size 8\,112 alloc'd ==15004== at 0x4023F50: malloc (vg_replace_malloc.c:236) ==15004== by 0x82D4537: Perl_safesysmalloc (util.c:149) ==15004== by 0x849BCE0: Perl_new_stackinfo (scope.c:64) ==15004== by 0x80F6E8C: Perl_init_stacks (perl.c:4055) ==15004== by 0x80FABB0: perl_construct (perl.c:249) ==15004== by 0x8062C1F: main (perlmain.c:110) ==15004== ==15004== Invalid read of size 1 ==15004== at 0x402592B: memcpy (mc_replace_strmem.c:497) ==15004== by 0x82ECD24: Perl_repeatcpy (string3.h:52) ==15004== by 0x8477653: Perl_pp_repeat (pp.c:1744) ==15004== by 0x836EABF: Perl_runops_standard (run.c:41) ==15004== by 0x810ABD2: perl_run (perl.c:2448) ==15004== by 0x8062C44: main (perlmain.c:116) ==15004== Address 0x4217a15 is 3 bytes before a block of size 8\,112 alloc'd ==15004== at 0x4023F50: malloc (vg_replace_malloc.c:236) ==15004== by 0x82D4537: Perl_safesysmalloc (util.c:149) ==15004== by 0x849BCE0: Perl_new_stackinfo (scope.c:64) ==15004== by 0x80F6E8C: Perl_init_stacks (perl.c:4055) ==15004== by 0x80FABB0: perl_construct (perl.c:249) ==15004== by 0x8062C1F: main (perlmain.c:110) ==15004== ==15004== Invalid read of size 1 ==15004== at 0x4025934: memcpy (mc_replace_strmem.c:497) ==15004== by 0x82ECD24: Perl_repeatcpy (string3.h:52) ==15004== by 0x8477653: Perl_pp_repeat (pp.c:1744) ==15004== by 0x836EABF: Perl_runops_standard (run.c:41) ==15004== by 0x810ABD2: perl_run (perl.c:2448) ==15004== by 0x8062C44: main (perlmain.c:116) ==15004== Address 0x4217a14 is 4 bytes before a block of size 8\,112 alloc'd ==15004== at 0x4023F50: malloc (vg_replace_malloc.c:236) ==15004== by 0x82D4537: Perl_safesysmalloc (util.c:149) ==15004== by 0x849BCE0: Perl_new_stackinfo (scope.c:64) ==15004== by 0x80F6E8C: Perl_init_stacks (perl.c:4055) ==15004== by 0x80FABB0: perl_construct (perl.c:249) ==15004== by 0x8062C1F: main (perlmain.c:110) ==15004== ==15004== ==15004== Process terminating with default action of signal 11 (SIGSEGV) ==15004== Access not within mapped region at address 0x47D784B ==15004== at 0x402591F: memcpy (mc_replace_strmem.c:497) ==15004== by 0x82ECD24: Perl_repeatcpy (string3.h:52) ==15004== by 0x8477653: Perl_pp_repeat (pp.c:1744) ==15004== by 0x836EABF: Perl_runops_standard (run.c:41) ==15004== by 0x810ABD2: perl_run (perl.c:2448) ==15004== by 0x8062C44: main (perlmain.c:116) ==15004== If you believe this happened as a result of a stack ==15004== overflow in your program's main thread (unlikely but ==15004== possible)\, you can try to increase the size of the ==15004== main thread stack using the --main-stacksize= flag. ==15004== The main thread stack size used in this run was 8388608. ==15004== Invalid free() / delete / delete[] ==15004== at 0x4023B6A: free (vg_replace_malloc.c:366) ==15004== by 0x41D68B4: _nl_archive_subfreeres (in /lib/i686/cmov/libc-2.11.3.so) ==15004== by 0x41D6506: free_mem (in /lib/i686/cmov/libc-2.11.3.so) ==15004== by 0x41D6D79: __libc_freeres (in /lib/i686/cmov/libc-2.11.3.so) ==15004== by 0x401F4D3: _vgnU_freeres (vg_preloaded.c:62) ==15004== by 0x44F784B: ??? ==15004== by 0x82ECD24: Perl_repeatcpy (string3.h:52) ==15004== by 0x8477653: Perl_pp_repeat (pp.c:1744) ==15004== by 0x836EABF: Perl_runops_standard (run.c:41) ==15004== by 0x810ABD2: perl_run (perl.c:2448) ==15004== by 0x8062C44: main (perlmain.c:116) ==15004== Address 0x422a3ac is 780 bytes inside a block of size 4\,080 alloc'd ==15004== at 0x4023F50: malloc (vg_replace_malloc.c:236) ==15004== by 0x82D4537: Perl_safesysmalloc (util.c:149) ==15004== by 0x83A166F: S_more_sv (sv.c:304) ==15004== by 0x83F01BC: Perl_newSVuv (sv.c:9247) ==15004== by 0x831C1E4: Perl_boot_core_mro (mro_core.c:128) ==15004== by 0x8113DBD: S_parse_body (perl.c:2198) ==15004== by 0x8117B1E: perl_parse (perl.c:1626) ==15004== by 0x80629D8: main (perlmain.c:114) ==15004== vex x86->IR: unhandled instruction bytes: 0xF0 0xC0 0x22 0x4 ==15004== ==15004== Process terminating with default action of signal 11 (SIGSEGV) ==15004== Bad permissions for mapped region at address 0x420CFF4 ==15004== at 0x4214CC3: ??? ==15004== by 0x41D6506: free_mem (in /lib/i686/cmov/libc-2.11.3.so) ==15004== by 0x41D6D79: __libc_freeres (in /lib/i686/cmov/libc-2.11.3.so) ==15004== by 0x401F4D3: _vgnU_freeres (vg_preloaded.c:62) ==15004== by 0x44F784B: ??? ==15004== by 0x82ECD24: Perl_repeatcpy (string3.h:52) ==15004== by 0x8477653: Perl_pp_repeat (pp.c:1744) ==15004== by 0x836EABF: Perl_runops_standard (run.c:41) ==15004== by 0x810ABD2: perl_run (perl.c:2448) ==15004== by 0x8062C44: main (perlmain.c:116) ==15004== ==15004== HEAP SUMMARY: ==15004== in use at exit: 92\,716 bytes in 613 blocks ==15004== total heap usage: 736 allocs\, 124 frees\, 100\,926 bytes allocated ==15004== ==15004== LEAK SUMMARY: ==15004== definitely lost: 15\,300 bytes in 433 blocks ==15004== indirectly lost: 0 bytes in 0 blocks ==15004== possibly lost: 12\,910 bytes in 140 blocks ==15004== still reachable: 64\,506 bytes in 40 blocks ==15004== suppressed: 0 bytes in 0 blocks ==15004== Rerun with --leak-check=full to see details of leaked memory ==15004== ==15004== For counts of detected and suppressed errors\, rerun with: -v ==15004== ERROR SUMMARY: 5527070 errors from 9 contexts (suppressed: 25 from 8) Segmentation fault

As you can see\, things get all sorts of pear-shaped after that batch of illegal accesses.

p5pRT commented 9 years ago

From @tonycoz

On Wed Sep 02 07:56:01 2015\, davem wrote:

On Fri\, Aug 28\, 2015 at 09:12:12PM -0700\, Dan Collins wrote:

f$0(d..M)x{}

NB: f$0(a..n)x{} (a 14-item list) also fails with this behavior\, a..m

Unfortunately I can't reproduce this. It's made harder by the fact that the anon hash {} is being used in a numeric context as the repeat count\, and a ref in numeric context returns the address of the referent\, e.g.

I suspect you're not testing on a 32-bit machine (or VM).

For the case where I see the crash:

1724 if (count > 1) { (gdb) p count $1 = 138580584 (gdb) p items $2 = 23 (gdb) p ~0 $3 = -1 (gdb) p (unsigned)~0 $4 = 4294967295 (gdb) p (unsigned)~0/count $5 = 30 (gdb) n 1727 if ( items > MEM_SIZE_MAX / (UV)count /* max would overflow */ (gdb) 1728 || items > (U32)I32_MAX / sizeof(SV *) /* repeatcpy would overflow */ (gdb) 1731 max = items * count; (gdb) 1732 MEXTEND(MARK\, max); (gdb) p max $6 = 3187353432 (gdb) p (int)max $7 = -1107613864

max is outside the range of SSize_t\, and MEXTEND() treats the supplied value as a SSize_t\, so the test in MEXTEND results in no stack growth.

Changing the test in pp_repeat to:

if ( items > SSize_t_MAX / (UV)count /* max would overflow */ || items > (U32)I32_MAX / sizeof(SV *) /* repeatcpy would overflow */ ) Perl_croak(aTHX_ "%s"\,"Out of memory during list extend");

produces a reasonable:

[tony@localhost perl]$ ./perl -e 'f$0(d..M)x{}' Out of memory during list extend at -e line 1.

Tony

p5pRT commented 9 years ago

From @iabyn

On Wed\, Sep 02\, 2015 at 10:17:03PM -0700\, Tony Cook via RT wrote:

On Wed Sep 02 07:56:01 2015\, davem wrote:

On Fri\, Aug 28\, 2015 at 09:12:12PM -0700\, Dan Collins wrote:

f$0(d..M)x{}

NB: f$0(a..n)x{} (a 14-item list) also fails with this behavior\, a..m

Unfortunately I can't reproduce this. It's made harder by the fact that the anon hash {} is being used in a numeric context as the repeat count\, and a ref in numeric context returns the address of the referent\, e.g.

I suspect you're not testing on a 32-bit machine (or VM).

Yeah\, I spotted that a bit later. I've been working on a general fix but a cold got in the way :-(

-- You live and learn (although usually you just live).

p5pRT commented 9 years ago

From @iabyn

On Wed\, Sep 02\, 2015 at 10:17:03PM -0700\, Tony Cook via RT wrote:

For the case where I see the crash: [snip] max is outside the range of SSize_t\, and MEXTEND() treats the supplied value as a SSize_t\, so the test in MEXTEND results in no stack growth.

I think the deeper bug is that MEXTEND() (and EXTEND()\, and various other stack/array extending macros and functions) take a signed count\, and don't handle a negative value very well\, either because the caller passed a negative signed value\, or passed a large unsigned value that became negative when integral conversion was performed.

So I think that pp_repeat() is wrong for passing an unsigned value to MEXTEND()\, which is documented to have a SSize_t arg\, but that MEXTEND etc need fixing too. pp_repeat() is just a particularly easy way to trigger the defect in the macros.

Currently the core of EXTEND/MEXTEND look like:

if (UNLIKELY(PL_stack_max - p \< (SSize_t)(n))) sp = stack_grow(sp\,p\,(SSize_t) (n));

I think the two things wrong with that are that the casts just quietly hide any issues (including truncating when sizeof(void*) \< sizeof(long)\, and that it doesn't check for overflow/underflow.

I can see two approaches to fixing it (and similar macros). Both approaches add a second check. The first would be:

if (UNLIKELY((n) \< 0 || p + (n) > PL_stack_max)) { sp = stack_grow(sp\,p\,(n));

and the second

SV ** const newp = p + (n); if (UNLIKELY(newp \ PL_stack_max)) { sp = stack_grow(sp\,p\,(n));

They are both more or less equivalent; the major difference is that the first will produce lots of compiler warnings when callers use an unsigned arg. This may be regarded as a good thing as it may help XS Authors flush out bugs. On the other hand it could could be regarded as a bad thing as it makes lots of XS modules noisy (and some compilers may complain that something always evaluates to true when the arg is literal\, eg "if ((3 \< 0) || ....)".

I'd be interested in opinions on which approach is best (or if there's a third approach).

In either case\, this change just means that stack_grow() is now called on negative size\, so it needs a new check and panic. A similar issue applies to av_extend_guts(). The following diff passes all tests. Note that it flushed out one bug: pp_aassign() was sometimes calling av_extend(av\,-2).

Inline Patch

```diff diff --git a/av.c b/av.c index cb99ceb..2c4740b 100644 --- a/av.c +++ b/av.c @@ -87,6 +87,10 @@ Perl_av_extend_guts(pTHX_ AV *av, SSize_t key, SSize_t *maxp, SV ***allocp, { PERL_ARGS_ASSERT_AV_EXTEND_GUTS; + if (key < -1) /* -1 is legal */ + Perl_croak(aTHX_ + "panic: av_extend_guts() negative count (%"IVdf")", (IV)key); + if (key > *maxp) { SV** ary; SSize_t tmp; diff --git a/pp.h b/pp.h index 2d99a72..78228a0 100644 --- a/pp.h +++ b/pp.h @@ -285,27 +285,29 @@ Does not use C. See also C>, C> and C>. #ifdef STRESS_REALLOC # define EXTEND(p,n) STMT_START { \ - sp = stack_grow(sp,p,(SSize_t) (n)); \ + sp = stack_grow(sp,p,(n)); \ PERL_UNUSED_VAR(sp); \ } STMT_END /* Same thing, but update mark register too. */ # define MEXTEND(p,n) STMT_START { \ const SSize_t markoff = mark - PL_stack_base; \ - sp = stack_grow(sp,p,(SSize_t) (n)); \ + sp = stack_grow(sp,p,(n)); \ mark = PL_stack_base + markoff; \ PERL_UNUSED_VAR(sp); \ } STMT_END #else # define EXTEND(p,n) STMT_START { \ - if (UNLIKELY(PL_stack_max - p < (SSize_t)(n))) { \ - sp = stack_grow(sp,p,(SSize_t) (n)); \ + SV ** const newp = p + (n); \ + if (UNLIKELY(newp PL_stack_max)) { \ + sp = stack_grow(sp,p,(n)); \ PERL_UNUSED_VAR(sp); \ } } STMT_END /* Same thing, but update mark register too. */ # define MEXTEND(p,n) STMT_START { \ - if (UNLIKELY(PL_stack_max - p < (SSize_t)(n))) { \ + SV ** const newp = p + (n); \ + if (UNLIKELY(newp PL_stack_max)) { \ const SSize_t markoff = mark - PL_stack_base; \ - sp = stack_grow(sp,p,(SSize_t) (n)); \ + sp = stack_grow(sp,p,(n)); \ mark = PL_stack_base + markoff; \ PERL_UNUSED_VAR(sp); \ } } STMT_END diff --git a/pp_hot.c b/pp_hot.c index bed0a27..639abc4 100644 --- a/pp_hot.c +++ b/pp_hot.c @@ -1275,7 +1275,9 @@ PP(pp_aassign) } av_clear(ary); - av_extend(ary, lastrelem - relem); + if (relem <= lastrelem) + av_extend(ary, lastrelem - relem); + i = 0; while (relem <= lastrelem) { /* gobble up all the rest */ SV **didstore; diff --git a/scope.c b/scope.c index 9768c30..1b89186 100644 --- a/scope.c +++ b/scope.c @@ -31,6 +31,10 @@ Perl_stack_grow(pTHX_ SV **sp, SV **p, SSize_t n) { PERL_ARGS_ASSERT_STACK_GROW; + if (n < 0) + Perl_croak(aTHX_ + "panic: stack_grow() negative count (%"IVdf")", (IV)n); + PL_stack_sp = sp; #ifndef STRESS_REALLOC av_extend(PL_curstack, (p - PL_stack_base) + (n) + 128); -- ```

My get-up-and-go just got up and went.

p5pRT commented 9 years ago

From preaction@me.com

On Sep 7\, 2015\, at 9:31 AM\, Dave Mitchell \davem@iabyn\.com wrote:

On Wed\, Sep 02\, 2015 at 10:17:03PM -0700\, Tony Cook via RT wrote:

For the case where I see the crash: [snip] max is outside the range of SSize_t\, and MEXTEND() treats the supplied value as a SSize_t\, so the test in MEXTEND results in no stack growth.

I think the deeper bug is that MEXTEND() (and EXTEND()\, and various other stack/array extending macros and functions) take a signed count\, and don't handle a negative value very well\, either because the caller passed a negative signed value\, or passed a large unsigned value that became negative when integral conversion was performed.

Does that mean this is related to XSRETURN() allowing negative values? Will fixing this fix that?

Old thread: http://www.nntp.perl.org/group/perl.perl5.porters/2015/06/msg228284.html \<http://www.nntp.perl.org/group/perl.perl5.porters/2015/06/msg228284.html>

p5pRT commented 9 years ago

From @iabyn

On Mon\, Sep 07\, 2015 at 02:29:10PM -0500\, Douglas Bell wrote:

Does that mean this is related to XSRETURN() allowing negative values? Will fixing this fix that?

Old thread: http://www.nntp.perl.org/group/perl.perl5.porters/2015/06/msg228284.html \<http://www.nntp.perl.org/group/perl.perl5.porters/2015/06/msg228284.html>

Probably. I'll review your stuff while I'm at it.

-- This is a great day for France! -- Nixon at Charles De Gaulle's funeral

p5pRT commented 9 years ago

From @tonycoz

On Mon Sep 07 07:32:01 2015\, davem wrote:

So I think that pp_repeat() is wrong for passing an unsigned value to MEXTEND()\, which is documented to have a SSize_t arg\, but that MEXTEND etc need fixing too. pp_repeat() is just a particularly easy way to trigger the defect in the macros.

Currently the core of EXTEND/MEXTEND look like:

if (UNLIKELY(PL_stack_max - p \< (SSize_t)(n))) sp = stack_grow(sp\,p\,(SSize_t) (n));

I think the two things wrong with that are that the casts just quietly hide any issues (including truncating when sizeof(void*) \< sizeof(long)\, and that it doesn't check for overflow/underflow.

I can see two approaches to fixing it (and similar macros). Both approaches add a second check. The first would be:

if (UNLIKELY((n) \< 0 || p + (n) > PL_stack_max)) { sp = stack_grow(sp\,p\,(n));

This seems safe\, though the warnings will be an issue.

and the second

SV ** const newp = p + (n); if (UNLIKELY(newp \ PL_stack_max)) { sp = stack_grow(sp\,p\,(n));

This is unsafe: if n * sizeof(SV*) is around MEM_SIZE then newp might wrap and point into the middle of the allocated stack\, avoiding the stack_grow() call. Such pointer wrapping is also undefined behaviour.

In either case\, this change just means that stack_grow() is now called on negative size\, so it needs a new check and panic. A similar issue applies to av_extend_guts(). The following diff passes all tests. Note that it flushed out one bug: pp_aassign() was sometimes calling av_extend(av\,- 2).

diff --git a/av.c b/av.c index cb99ceb..2c4740b 100644 --- a/av.c +++ b/av.c @@ -87\,6 +87\,10 @@ Perl_av_extend_guts(pTHX_ AV *av\, SSize_t key\, SSize_t *maxp\, SV ***allocp\, { PERL_ARGS_ASSERT_AV_EXTEND_GUTS;

+ if (key \< -1) /* -1 is legal */ + Perl_croak(aTHX_ + "panic: av_extend_guts() negative count (%"IVdf")"\, (IV)key); + if (key > *maxp) { SV** ary; SSize_t tmp; ...

Makes sense to me.

Tony

p5pRT commented 9 years ago

From @iabyn

On Thu\, Sep 10\, 2015 at 06:21:49PM -0700\, Tony Cook via RT wrote:

if (UNLIKELY((n) \< 0 || p + (n) > PL_stack_max)) { sp = stack_grow(sp\,p\,(n));

This seems safe\, though the warnings will be an issue.

Thinking about it further\, that's not safe either. For example if p and n are both 32-bit say\, and where p = 2.5Gb\, max = 2.6Gb and n = 1.9Gb\, then p+n will wrap to 0.4Gb and the stack grow will be skipped.

I'm tempted to go back to the original scheme\, but without the cast; i.e. change

PL_stack_max - sp \< (SSize_t)(n)

to

PL_stack_max - sp \< (n)

As far as I can see this is completely safe. The only possible wrap is if the current spare allocated stack is > 2GB in size\, where PL_stack_max - sp would go negative on a 32-bit system\, but this just (at worst) gives a false positive\, and calls grow_stack() anyway.

The downside of it is that the compiler will complain about "comparison between signed and unsigned integer expressions" if the n passed to EXTEND() is an *unsigned* int type. But this could be regarded as a Good Thing. EXTEND() is documented to take a signed value (originally int\, latterly Ssize_t)\, so perhaps XS code that passes an unsigned value should warn. Changing this in bleed gives about 4 warnings in core\, 4 in ext/ and one in cpan/.

Does anyone think that warning would be a bad thing?

-- Hofstadter's Law: It always takes longer than you expect\, even when you take into account Hofstadter's Law.

p5pRT commented 9 years ago

From @bulk88

On Mon Sep 07 07:32:01 2015\, davem wrote:

Currently the core of EXTEND/MEXTEND look like:

if (UNLIKELY(PL_stack_max - p \< (SSize_t)(n))) sp = stack_grow(sp\,p\,(SSize_t) (n));

I think the two things wrong with that are that the casts just quietly hide any issues (including truncating when sizeof(void*) \< sizeof(long)\, and that it doesn't check for overflow/underflow.

I can see two approaches to fixing it (and similar macros). Both approaches add a second check. The first would be:

if (UNLIKELY((n) \< 0 || p + (n) > PL_stack_max)) { sp = stack_grow(sp\,p\,(n));

and the second

SV ** const newp = p + (n); if (UNLIKELY(newp \ PL_stack_max)) { sp = stack_grow(sp\,p\,(n));

They are both more or less equivalent; the major difference is that the first will produce lots of compiler warnings when callers use an unsigned arg. This may be regarded as a good thing as it may help XS Authors flush out bugs. On the other hand it could could be regarded as a bad thing as it makes lots of XS modules noisy (and some compilers may complain that something always evaluates to true when the arg is literal\, eg "if ((3 \< 0) || ....)".

I'd be interested in opinions on which approach is best (or if there's a third approach).

In either case\, this change just means that stack_grow() is now called on negative size\, so it needs a new check and panic. A similar issue applies to av_extend_guts(). The following diff passes all tests. Note that it flushed out one bug: pp_aassign() was sometimes calling av_extend(av\,- 2).

# define EXTEND(p\,n) STMT_START { \ - if (UNLIKELY(PL_stack_max - p \< (SSize_t)(n))) { \ - sp = stack_grow(sp\,p\,(SSize_t) (n)); \ + SV ** const newp = p + (n); \ + if (UNLIKELY(newp \ PL_stack_max)) { \ + sp = stack_grow(sp\,p\,(n)); \ PERL_UNUSED_VAR(sp); \ } } STMT_END

Currently this code uses 1 branch. Now you want to make it 2 branches. EXTEND is high on the most commonly used function call/macro call list. My libperl has 174 calls for Perl_stack_grow. Any XSUB that returns more than 1 arg will have an Perl_stack_grow call in it (grep for PPCODE). A negative grow value is rare\, it should be handled inside Perl_stack_grow\, and if there is nothing to grow since "it is negative and shrinking not implemented" just silently return from Perl_stack_grow. If passing a negative grow count is determined to be an illegal argument\, on the theory that is must be an attempt at "out of memory" or a wrap\, then do a die inside Perl_stack_grow or just let av_extend deal with the negative number.

There is another problem that a very large/negative grow count in 32 bits will require the perl stack to be > (2^31*4) 8.5 GB long in a 4 GB memory space\, so something else will always error out eventually in the call stack\, if it doesn't the bug is in that something else\, the bug isn't in EXTEND macro.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 9 years ago

From @iabyn

On Sun\, Sep 20\, 2015 at 12:12:37PM -0700\, bulk88 via RT wrote:

Currently this code uses 1 branch. Now you want to make it 2 branches. EXTEND is high on the most commonly used function call/macro call list. My libperl has 174 calls for Perl_stack_grow. Any XSUB that returns more than 1 arg will have an Perl_stack_grow call in it (grep for PPCODE).

The vast bulk of those calls to EXTEND() have a literal constant N\, in which case the extra (n \< 0) constant folds away.

A negative grow value is rare\, it should be handled inside Perl_stack_grow\, and if there is nothing to grow since "it is negative and shrinking not implemented" just silently return from Perl_stack_grow. If passing a negative grow count is determined to be an illegal argument\, on the theory that is must be an attempt at "out of memory" or a wrap\, then do a die inside Perl_stack_grow or just let av_extend deal with the negative number.

But the whole point is that without the extra (n \< 0) test\, Perl_stack_grow() is never called. Its a false negative. The caller has screwed up and somehow ended up with a negative count\, e.g. through using inappropriate casts or types. On return it thinks the stack has been safely extended when it hasn't\, so proceeds to stomp all over the end of it.

There is another problem that a very large/negative grow count in 32 bits will require the perl stack to be > (2^31*4) 8.5 GB long in a 4 GB memory space\, so something else will always error out eventually in the call stack\, if it doesn't the bug is in that something else\, the bug isn't in EXTEND macro.

But it may well error out by stomping over the end of the stack which hasn't been extended.

Anyway\, my current code looks like this and seems to working well on both 32/64 and 64-bit platforms:

#define _EXTEND_SAFE_N(n) \ (sizeof(n) > sizeof(SSize_t) && ((SSize_t)(n) != (n)) ? -1 : (n))

# define _EXTEND_NEEDS_GROW(p\,n) ( (n) \< 0 || PL_stack_max - p \< (n))

# define EXTEND(p\,n) STMT_START { \ if (UNLIKELY(_EXTEND_NEEDS_GROW(p\,n))) { \ sp = stack_grow(sp\,p\,_EXTEND_SAFE_N(n)); \ PERL_UNUSED_VAR(sp); \ } } STMT_END /* Same thing\, but update mark register too. */ # define MEXTEND(p\,n) STMT_START { \ if (UNLIKELY(_EXTEND_NEEDS_GROW(p\,n))) { \ const SSize_t markoff = mark - PL_stack_base;\ sp = stack_grow(sp\,p\,_EXTEND_SAFE_N(n)); \ mark = PL_stack_base + markoff; \ PERL_UNUSED_VAR(sp); \ } } STMT_END #endif

-- "I do not resent criticism\, even when\, for the sake of emphasis\, it parts for the time with reality". -- Winston Churchill\, House of Commons\, 22nd Jan 1941.

p5pRT commented 9 years ago

From zefram@fysh.org

Dave Mitchell wrote:

The vast bulk of those calls to EXTEND() have a literal constant N\, in which case the extra (n \< 0) constant folds away.

You could use gcc's __builtin_constant_p() to finesse this\, only including conditionals that you know will constant-fold. Optimise separately for constant and non-constant parameters.

-zefram

p5pRT commented 9 years ago

From @iabyn

On Mon\, Sep 21\, 2015 at 03:29:03PM +0100\, Zefram wrote:

Dave Mitchell wrote:

The vast bulk of those calls to EXTEND() have a literal constant N\, in which case the extra (n \< 0) constant folds away.

You could use gcc's __builtin_constant_p() to finesse this\, only including conditionals that you know will constant-fold. Optimise separately for constant and non-constant parameters.

I don't understand how this will help.

EXTEND(p\,n)

needs to behave differently if n is negative.

So it needs to test for n being negative if n is a var\, and there is no opportunity to optimise this away\, since we need to do the test. If n is a constant\, then GCC will optimise away the n \< 0 test without us needing to do anything special. In neither case do I see how __builtin_constant_p would help?

e.g.

EXTEND(p\,n)

compiles to

if (n \< 0 || PL_max_sp - p \< 1) ...

and

EXTEND(p\,1)

compiles under -O to

if (PL_max_sp - p \< 1) ...

-- Spock (or Data) is fired from his high-ranking position for not being able to understand the most basic nuances of about one in three sentences that anyone says to him. -- Things That Never Happen in "Star Trek" #19

p5pRT commented 9 years ago

From zefram@fysh.org

Dave Mitchell wrote:

So it needs to test for n being negative if n is a var\, and there is no opportunity to optimise this away\, since we need to do the test.

But you have a choice as to whether to do that test inline or in the heavy-case function. That was bulk88's concern about bloating call sites.

                              In neither case do I see how

__builtin_constant_p would help?

There are three essentially different places where the logic can be performed: compile-time constant folding\, inline at runtime\, and in the heavy-case function at runtime. You have to make the choice between the out-of-line function and the call-site two manually; the compiler then determines what of the call-site stuff (in the macro expansion) can be constant-folded. So normally\, if you want to have the logic have a chance to be constant folded\, you have to put it at the call site. But if you know it's a variable\, so constant folding isn't an option\, you wouldn't necessarily choose to put it at the call site; maybe you'd rather have it factored out into the heavy function.

__builtin_constant_p() lets you split those cases\, making the manual choice differently based on whether you would get constant folding. It lets you locate the logic preferentially in constant folding and alternatively in the out-of-line function\, entirely avoiding bloating the runtime call site.

I haven't reached an opinion myself about whether that's a good approach in this case\, mind. It just occurred to me that this could avoid a conflict over which case is more important to optimise for\, by letting you have it both ways.

-zefram

p5pRT commented 9 years ago

From @iabyn

On Mon\, Sep 21\, 2015 at 05:12:23PM +0100\, Zefram wrote:

__builtin_constant_p() lets you split those cases\, making the manual choice differently based on whether you would get constant folding. It lets you locate the logic preferentially in constant folding and alternatively in the out-of-line function\, entirely avoiding bloating the runtime call site.

But if we avoid bloating the call site by putting the n\<0 test in the stack_grow() function\, then we *always* have to call stack_grow() in order to test if the var is negative\, which rather defeats the whole point of the EXTEND macro()\, which is to skip the call to stack_grow() 99.9% of the time.

-- "Foul and greedy Dwarf - you have eaten the last candle." -- "Hordes of the Things"\, BBC Radio.

p5pRT commented 9 years ago

From zefram@fysh.org

Dave Mitchell wrote:

But if we avoid bloating the call site by putting the n\<0 test in the stack_grow() function\, then we *always* have to call stack_grow() in order to test if the var is negative\,

Oh\, I see what you're getting at. I believe that for the purpose of determining whether to take the slow path you can fold the call-site n\<0 test into the existing \<n comparison\, by changing it to an unsigned comparison. __builtin_constant_p() would be relevant if you want to distinguish between n\<0 and stack-needs-to-grow (the two reasons for taking the slow path) with constant folding. Which on reflection doesn't seem as likely as I'd enthusiastically assumed.

-zefram

p5pRT commented 9 years ago

From @iabyn

On Tue\, Sep 22\, 2015 at 12:45:53PM +0100\, Zefram wrote:

Dave Mitchell wrote:

But if we avoid bloating the call site by putting the n\<0 test in the stack_grow() function\, then we *always* have to call stack_grow() in order to test if the var is negative\,

Oh\, I see what you're getting at. I believe that for the purpose of determining whether to take the slow path you can fold the call-site n\<0 test into the existing \<n comparison\, by changing it to an unsigned comparison.

But casting n to an unsigned type might truncate n if we choose too small a type\, and since EXTEND() is a macro\, we have no control over what type or expression the caller passes. This is rather how we got into the mess raised by this ticket in the first place.

I suppose we could cast n to a UV on the grounds that UV is guaranteed to to be as big as a pointer difference\, but that might still be bad if n was of some larger type (assuming that that is even possible/realistic).

Also\, I can't currently think of any reformulation of max - p \< n into something like map - p \< (UV)n that doesn't end up either doing a mixed signed/unsigned comparison (like that does)\, or end up doing something wrong or straying into C's unspecified/undefined territory; e.g. (UV)(max-p) \< (UV)n where if due to a logic error p happens to be > max\, will silently skip extending the stack.

Frankly C's int rules do my head in every time I go near them.

-- The optimist believes that he lives in the best of all possible worlds. As does the pessimist.

p5pRT commented 9 years ago

From @khwilliamson

On 09/22/2015 03:08 PM\, Dave Mitchell wrote:

But casting n to an unsigned type might truncate n if we choose too small a type\, and since EXTEND() is a macro\, we have no control over what type or expression the caller passes. This is rather how we got into the mess raised by this ticket in the first place.

I suppose we could cast n to a UV on the grounds that UV is guaranteed to to be as big as a pointer difference\, but that might still be bad if n was of some larger type (assuming that that is even possible/realistic).

I tried to solve this kind of issue in handy.h\, and there have been no complaints for several releases since 2011. It is

/* Specify the widest unsigned type on the platform. Use U64TYPE because U64 * is known only in the perl core\, and this macro can be called from outside * that */ #ifdef HAS_QUAD # define WIDEST_UTYPE U64TYPE #else # define WIDEST_UTYPE U32 #endif

p5pRT commented 9 years ago

From zefram@fysh.org

Dave Mitchell wrote:

But casting n to an unsigned type might truncate n if we choose too small a type\,

True. There is (u)intmax_t\, but that might be a portability concern.

I suppose we could cast n to a UV on the grounds that UV is guaranteed to to be as big as a pointer difference\,

size_t would be the natural choice. But it is possible to have a larger integer type.

straying into C's unspecified/undefined territory; e\.g\.
(UV)(max-p) \< (UV)n where if due to a logic error p happens to be > max\, will silently skip extending the stack.

p can't be > max\, unless the stack is already blown. Going further wrong in that case doesn't matter. So casting max-p to the unsigned type is safe. (size_t)(max-p) \< (size_t)n is what I had in mind.

-zefram

p5pRT commented 9 years ago

From @iabyn

On Wed\, Sep 23\, 2015 at 10:55:53AM +0100\, Zefram wrote:

straying into C's unspecified/undefined territory; e\.g\.
(UV)(max-p) \< (UV)n where if due to a logic error p happens to be > max\, will silently skip extending the stack.
p can't be > max\, unless the stack is already blown. Going further wrong in that case doesn't matter. So casting max-p to the unsigned type is safe. (size_t)(max-p) \< (size_t)n is what I had in mind.

Looking further\, The way the EXTEND/MEXTEND interface is designed\, it is legal for p > max. p isn't the stack pointer; p is any old pointer the caller cares to pass to EXTEND()\, saying "please make sure there are at least N slots above p".

For example in pp_leavesub()\, in the scalar context when there are no args on the stack to return\, it needs to push &PL_sv_undef; at that point MARK is the address where it wants to store the &PL_sv_undef. MARK is thus one above the current top of stack; pp_leavesub() quite legally\, if somewhat confusingly\, does:

MEXTEND(MARK\,0)

which has the net effect of extending the stack by one if necessary.

At this point I'm strongly minded to keep the additional n \< 0 condition; I haven't been been convinced that there is a safe way to avoid it\, and it typically will only remain in the binary for XS code which either calls with\, or returns\, a variable number of args\, which will probably have required a whole bunch of SV creates or mortal copies in a loop. The extra overhead to the XS call or return of the n\<0 test in this case will be infinitesimal.

-- A major Starfleet emergency breaks out near the Enterprise\, but fortunately some other ships in the area are able to deal with it to everyone's satisfaction. -- Things That Never Happen in "Star Trek" #13

p5pRT commented 9 years ago

From zefram@fysh.org

Dave Mitchell wrote:

For example in pp_leavesub()\, in the scalar context when there are no args on the stack to return\, it needs to push &PL_sv_undef; at that point MARK is the address where it wants to store the &PL_sv_undef. MARK is thus one above the current top of stack;

Eep. *That* invokes undefined behaviour. But as long as we have it\, you're right that p\<=max can't be assumed.

-zefram

p5pRT commented 9 years ago

From @iabyn

I've now pushed the following branch for smoking:

smoke-me/davem/extend

I've heavily tested against gcc\,g++\,clang on 32-bit\, 32/64-bit and 64-bit linux. It fixes the original 'x' operator bug amongst everything else.

It contains the following commits\, along with Doug's two XSRETURN() patches:

commit c319c248a6de750a9b66def437aae2a20041c1e2 Author: David Mitchell \davem@iabyn\.com AuthorDate: Mon Sep 21 14:49:22 2015 +0100 Commit: David Mitchell \davem@iabyn\.com CommitDate: Thu Sep 24 10:57:22 2015 +0100

fix up EXTEND() callers
The previous commit made it clear that the N argument to EXTEND() is supposed to be signed\, in particular SSize_t\, and now typically triggers compiler warnings where this isn't the case.
This commit fixes the various places in core that passed the wrong sort of N to EXTEND(). The fixes are in three broad categories.
First\, where sensible\, I've changed the relevant var to be SSize_t.
Second\, where its expected that N could never be large enough to wrap\, I've just added an assert and a cast.
Finally\, I've added extra code to detect whether the cast could wrap/truncate\, and if so set N to -1\, which will trigger a panic in stack_grow().
This also fixes [perl #125937] 'x' operator on list causes segfault with possible stack corruption

commit 882096609dada9e1e799b27eb354b013a8393d96 Author: David Mitchell \davem@iabyn\.com AuthorDate: Mon Sep 7 15:00:32 2015 +0100 Commit: David Mitchell \davem@iabyn\.com CommitDate: Thu Sep 24 10:57:22 2015 +0100

make EXTEND() and stack_grow() safe(r)
This commit fixes various issues around stack_grow() and its two main wrappers\, EXTEND() and MEXTEND(). In particular it behaves very badly on systems with 32-bit pointers but 64-bit ints.
One noticeable effect of this is commit is that various usages of EXTEND() etc will now start to give compiler warnings - usually because they're passing an unsigned N arg when it should be signed. This may indicate a logic error in the caller's code which needs fixing. This commit causes several such warnings to appear in core code\, which will be fixed in the next commit.
Essentially there are several potential false negatives in this basic code:
if (PL_stack_max - p \< (SSize_t)(n)) stack_grow(sp\,p\,(SSize_t)(n));
where it incorrectly skips the call to stack_grow() and then the caller tramples over the end of the stack because it assumes that it has in fact been extended. The value of N passed to stack_grow() can also potentially get truncated or wrapped.
Note that the N arg of stack_grow() is SSize_t and EXTEND()'s N arg is documented as SSize_t. In earlier times\, they were both ints. Significantly\, this means that they are both signed\, and always have been.
In detail\, the problems and their solutions are:
1) N is a signed value: if negative\, it could be an indication of a caller's invalid logic or wrapping in the caller's code. This should trigger a panic. Make it so by adding an extra test to EXTEND() to always call stack_grow if negative\, then add a check and panic in stack_grow() (and other places too). This extra test will be constant folded when EXTEND() is called with a literal N.
2) If the caller passes an unsigned value of N\, then the comparison is between a signed and an unsigned value\, leading to potential wrap-around. Casting N to SSize_t merely hides any compiler warnings\, thus failing to alert the caller to a problem with their code. In addition\, where sizeof(N) > sizeof(SSize_t)\, the cast may truncate N\, again leading to false negatives. The solution is to remove the cast\, and let the caller deal with any compiler warnings that result.
3) Similarly\, casting stack_grow()'s N arg can hide any warnings issued by e.g. -Wconversion. So remove it. It still does the wrong thing if the caller uses a non-signed type (usually a panic in stack_grow())\, but coders have slightly more chance of spotting issues at compile time now.
4) If sizeof(N) > sizeof(SSize_t)\, then the N arg to stack_grow() may get truncated or sign-swapped. Add a test for this (basically that N is too big to fit in a SSize_t); for simplicity\, in this case just set N to -1 so that stack_grow() panics shortly afterwards. In platforms where this can't happen\, the test is constant folded away.
With all these changes\, the macro now looks in essence like:
if ( n \< 0 || PL_stack_max - p \< n) stack_grow(sp\,p\, (sizeof(n) > sizeof(SSize_t) && ((SSize_t)(n) != n) ? -1 : n));

commit f25d48af06678fb44eab3850be2f6b6c6e44b918 Author: David Mitchell \davem@iabyn\.com AuthorDate: Wed Sep 9 13:02:40 2015 +0100 Commit: David Mitchell \davem@iabyn\.com CommitDate: Thu Sep 24 10:57:22 2015 +0100

fix some 32/64-bit compiler warnings
Some bits of code don't do well on a 32-bit system with 64-bit ints (-Duse64bitint)
In particular:
_MEM_WRAP_NEEDS_RUNTIME_CHECK: if sizeof(MEM_SIZE) > sizeof(n)\, then the shift count could be negative
S_regmatch: ln and n were two different sizes and signesses\, so comparing them warned. Since they were being mis-used as two convenient temporary booleans anyway\, just use temporary booleans instead.
Perl_sv_vcatpvfn_flags: the test/assertion (IV)elen \< 0 was (I think) being used to test for signed/unsigned conversion wrap-around. elen is of type STRLEN which is a pointer-based type\, so can be 32-bit while IV is 64-bit. Instead compare it to half the maximum value of a STRLEN var to see if it may have wrapped.

-- "There's something wrong with our bloody ships today\, Chatfield." -- Admiral Beatty at the Battle of Jutland\, 31st May 1916.

p5pRT commented 9 years ago

From @bulk88

On Thu Sep 24 09:52:00 2015\, davem wrote:

I've now pushed the following branch for smoking:

smoke-me/davem/extend

There is something wrong looking about this branch. 32 IV 32 ptr VC 2003 -O1 perl. C "int" is 32 bits on windows.

given

PADNAME * Perl_newPADNAMEpvn(const char *s\, STRLEN len) { struct padname_with_str *alloc; char *alloc2; /* for Newxz */ PADNAME *pn; PERL_ARGS_ASSERT_NEWPADNAMEPVN; Newxz(alloc2\, STRUCT_OFFSET(struct padname_with_str\, xpadn_str[0]) + len + 1\, char); alloc = (struct padname_with_str *)alloc2; pn = (PADNAME *)alloc; PadnameREFCNT(pn) = 1; PadnamePV(pn) = alloc->xpadn_str; Copy(s\, PadnamePV(pn)\, len\, char); *(PadnamePV(pn) + len) = '\0'; PadnameLEN(pn) = len; return pn; }

before at e120c24fe257993e9cbf4c567194bec2792f3ccc

disassebled from perl523.dll after VC -O1 optimization.

padname *__cdecl Perl_newPADNAMEpvn(const char *s\, unsigned int len) { void *v3; // eax@1 void *v4; // esi@1

v3 = Perl_safesyscalloc(len + 31\, 1u); v4 = v3; *((_DWORD *)v3 + 5) = 1; *(_DWORD *)v3 = (char *)v3 + 30; memcpy((char *)v3 + 30\, s\, len); *(_BYTE *)(len + *(_DWORD *)v4) = 0; *((_BYTE *)v4 + 28) = len; return (padname *)v4; }

after at 7a36e618ad808bf649080137e3fb56386d8420e3

padname *__cdecl Perl_newPADNAMEpvn(const char *s\, unsigned int len) { padname *result; // eax@2 void *v3; // eax@3 void *v4; // esi@3 void *v5; // eax@3

if ( len + 31 \<= 0xFFFFFFFF ) { v5 = Perl_safesyscalloc(len + 31\, 1u); v4 = v5; v3 = (char *)v5 + 30; *((_DWORD *)v4 + 5) = 1; *(_DWORD *)v4 = v3; if ( len \<= 0xFFFFFFFF ) { memcpy(v3\, s\, len); *(_BYTE *)(len + *(_DWORD *)v4) = 0; *((_BYTE *)v4 + 28) = len; result = (padname *)v4; } else { result = (padname *)S_croak_memory_wrap(); } } else { result = (padname *)S_croak_memory_wrap(); } return result; }

There is something about " if ( len \<= 0xFFFFFFFF )" that looks wrong. It is part of the perlapi Copy macro. Maybe there is something I dont know about C\, but how can a U32 ever be biggger than 0xFFFFFFFF on x86-32?

The instructions behind that line are

.text:280B887A 83 FB FF cmp ebx\, 0FFFFFFFFh ***cut irrelavent instructions********* .text:280B888D 76 05 jbe short loc_280B8894 .text:280B888F E9 E2 1D F7 FF jmp S_croak_memory_wrap .text:280B8894 loc_280B8894: .text:280B8894 53 push ebx ; Size .text:280B8895 FF 74 24 10 push [esp+0Ch+s] ; Src .text:280B8899 50 push eax ; Dst .text:280B889A E8 AF AF 01 00 call _memcpy

The change is probably caused by this hunk.

* fix some 32/64-bit compiler warnings

f25d48af06678fb44eab3850be2f6b6c6e44b918 handy.h | 5 ++++- regexec.c | 48 ++++++++++++++++++++++++++++++------------------ sv.c | 10 ++++++---- 3 files changed\, 40 insertions(+)\, 23 deletions(-)

Inline Patch

```diff diff --git a/handy.h b/handy.h index 0318504..b30bdf5 100644 --- a/handy.h +++ b/handy.h @@ -1917,10 +1917,13 @@ PoisonWith(0xEF) for catching access to freed memory. * As well as avoiding the need for a run-time check in some cases, it's * designed to avoid compiler warnings like: * comparison is always false due to limited range of data type + * It's mathematically equivalent to + * max(n) * sizeof(t) > MEM_SIZE_MAX */ # define _MEM_WRAP_NEEDS_RUNTIME_CHECK(n,t) \ - (sizeof(t) > ((MEM_SIZE)1 << 8*(sizeof(MEM_SIZE) - sizeof(n)))) + ( sizeof(MEM_SIZE) <= sizeof(n) \ + || sizeof(t) > ((MEM_SIZE)1 << 8*(sizeof(MEM_SIZE) - sizeof(n)))) ```

.text section size of perl523.dll before is 0xd2743 bytes. After, 0xd3113 bytes. A delta of 2512 bytes. That number looks like bloat in every caller, not checks being moved to inside the core C function inside a macro (the check/test and croak in Perl_stack_grow is fine).

Where is the bloat coming from? I attached a diff of function sizes to this post.

To nit pick (I personally dont care)\, I also have a suspicion (I didnt step it) that the EXTEND negative check wont catch this and will wrap on 32 ptr 32 IV perl and do something wrong after +1 or +128 is added in Perl_stack_grow.

EXTEND(SP\, IV_MAX);

An slightly related issue. EXTEND macro and Perl_stack_grow have a badly design prototype. Perl_stack_grow supports the concept of multiple stack pointers with different C auto names. That is why args SV** sp and SV ** p exist. I've never seen this used this feature anymore. It looks like CPAN will be just fine with EXTEND macro ignoring the 1st argument always assuming it was SP with a note in perlapi that the arg is ignored but can't be removed for historical reasons. EXTENDSP(n) is a name for a replacement for EXTEND. Or EXTENDST or for less typing EXTENDS(n)\, S=stack.

http://grep.cpan.me/?q=\WEXTEND\%28[^Ss]

Greping shows 1 line in perl core where the arg isnt SP or sp.

https://metacpan.org/source/RJBS/perl-5.22.0/pp_ctl.c#L2339

2 lines on CPAN in 1 module\, but that module is already deeply non-public API with using POPSTACK like its perl core and highly likely to break (and is broken on 5.22 http://matrix.cpantesters.org/?dist=Params-Lazy+0.005 ).

https://metacpan.org/source/HUGMEIR/Params-Lazy-0.005/Lazy.xs#L667

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 9 years ago

From @bulk88

davemstackfncdelta.txt

p5pRT commented 9 years ago

From @iabyn

On Thu\, Sep 24\, 2015 at 04:10:33PM -0700\, bulk88 via RT wrote:

smoke-me/davem/extend

There is something wrong looking about this branch. 32 IV 32 ptr VC 2003 -O1 perl. C "int" is 32 bits on windows. [snip] The change is probably caused by this hunk. [snip] # define _MEM_WRAP_NEEDS_RUNTIME_CHECK(n\,t) \ - (sizeof(t) > ((MEM_SIZE)1 \<\< 8*(sizeof(MEM_SIZE) - sizeof(n)))) + ( sizeof(MEM_SIZE) \<= sizeof(n) \ + || sizeof(t) > ((MEM_SIZE)1 \<\< 8*(sizeof(MEM_SIZE) - sizeof(n))))

Thanks for spotting this. The '\<=' should have been '\<'. It turns out that gcc still manages to constant fold the run-time expression\, so it wasn't obvious here.

I've now pushed extend2 for smoking with this change (plus a fix for Doug's XSRETURN_NV(0.1) issue). Can you confirm that fixes it?

To nit pick (I personally dont care)\, I also have a suspicion (I didnt step it) that the EXTEND negative check wont catch this and will wrap on 32 ptr 32 IV perl and do something wrong after +1 or +128 is added in Perl_stack_grow.

EXTEND(SP\, IV_MAX);

My original branch tests for negativity in Perl_av_extend_guts() too\, so in theory that should be caught. Which is not to say there aren't other issues lurking around as stack_grow() calls av_extend() which calls av_extend_guts() with different meanings for its args.

An slightly related issue. EXTEND macro and Perl_stack_grow have a badly design prototype. Perl_stack_grow supports the concept of multiple stack pointers with different C auto names. That is why args SV** sp and SV ** p exist. I've never seen this used this feature anymore. It looks like CPAN will be just fine with EXTEND macro ignoring the 1st argument always assuming it was SP with a note in perlapi that the arg is ignored but can't be removed for historical reasons. EXTENDSP(n) is a name for a replacement for EXTEND. Or EXTENDST or for less typing EXTENDS(n)\, S=stack.

http://grep.cpan.me/?q=\WEXTEND\%28[^Ss]

Greping shows 1 line in perl core where the arg isnt SP or sp.

https://metacpan.org/source/RJBS/perl-5.22.0/pp_ctl.c#L2339

2 lines on CPAN in 1 module\, but that module is already deeply non-public API with using POPSTACK like its perl core and highly likely to break (and is broken on 5.22 http://matrix.cpantesters.org/?dist=Params-Lazy+0.005 ).

https://metacpan.org/source/HUGMEIR/Params-Lazy-0.005/Lazy.xs#L667

Personally I've already spent more time on EXTEND() than I wanted too; If other people want to discuss/implement this that's fine by me\, but I don't want to be involved. I'll just mention however that there's also MEXTEND() to consider.

-- In England there is a special word which means the last sunshine of the summer. That word is "spring".

p5pRT commented 9 years ago

From @bulk88

On Fri Sep 25 03:33:53 2015\, davem wrote:

On Thu\, Sep 24\, 2015 at 04:10:33PM -0700\, bulk88 via RT wrote:

smoke-me/davem/extend

There is something wrong looking about this branch. 32 IV 32 ptr VC 2003 -O1 perl. C "int" is 32 bits on windows. [snip] The change is probably caused by this hunk. [snip] # define _MEM_WRAP_NEEDS_RUNTIME_CHECK(n\,t) \ - (sizeof(t) > ((MEM_SIZE)1 \<\< 8*(sizeof(MEM_SIZE) - sizeof(n)))) + ( sizeof(MEM_SIZE) \<= sizeof(n) \ + || sizeof(t) > ((MEM_SIZE)1 \<\< 8*(sizeof(MEM_SIZE) - sizeof(n))))

Thanks for spotting this. The '\<=' should have been '\<'. It turns out that gcc still manages to constant fold the run-time expression\, so it wasn't obvious here.

I've now pushed extend2 for smoking with this change (plus a fix for Doug's XSRETURN_NV(0.1) issue). Can you confirm that fixes it?

On davem/extend2 the test against 0xFFFFFFFF in Perl_newPADNAMEpvn are gone/optimized away so that problem was fixed. There is no difference in old Perl_newPADNAMEpvn and new Perl_newPADNAMEpvn now. There is 1 extra conditional jump per non-constant length EXTEND now (constant length EXTEND is still only 1 conditional jump). I guess nothing can be done about that.

before commit is SHA-1: ef058b33cd10c03cc5704ec5447df1d23fe3b48b

* POD fix in the documentation for SvTHINKFIRST

after commit on extend2 branch is

SHA-1: b32395ddb3477eff8bfb4505c99cef2407e6f1ce

* add tests for XSRETURN* macros

In most cases one extra conditional jump is to an existing label/code block\, it isn't introducing a new block of code which is from the "+# define _EXTEND_NEEDS_GROW(p\,n) ( (n) \< 0 || PL_stack_max - p \< (n))" part of the patch.

pp_stat went from

if ( (signed int)((char *)my_perl->Istack_max - (_DWORD)v2) >> 2 \< v49 ) v2 = Perl_stack_grow(my_perl\, v2\, v2\, v49);

to

if ( v49 \< 0 || (signed int)((char *)my_perl->Istack_max - (_DWORD)v2) >> 2 \< v49 ) v2 = Perl_stack_grow(my_perl\, v2\, v2\, v49);

pics attached of the extra jump.

.text section size of perl523.dll before/after on extend2 branch is 0xd27f3-0xd2743=0xb0\, 176 bytes\, more reasonable now\, compared to 2512 bytes of the original branch. Attached an updated func size delta.

There is one more strange thing about this branch but I need another hour to describe it so ill do it tomorrow.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 9 years ago

From @bulk88

On Sat Sep 26 06:08:10 2015\, bulk88 wrote:

pics attached of the extra jump.

Attachments didnt make it\, reposting.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 9 years ago

From @bulk88

--- C:\Documents and Settings\Owner\Desktop\predavemstack.txt +++ C:\Documents and Settings\Owner\Desktop\davemstack.txt @@ -529\,7 +529\,7 @@ S_ptr_hash 0000003E S_ptr_table_find 00000025 S_push_slab 00000041 -S_pushav 000000DA +S_pushav 000000DE S_qsortsv 0000014E S_qsortsvu 00000494 S_ref_array_or_hash 00000082 @@ -553\,7 +553\,7 @@ S_reghopmaybe3 0000005A S_reginclass 00000288 S_reginsert 000000BC -S_regmatch 00004AE4 +S_regmatch 00004AD4 S_regpatws 000000B0 S_regpiece 0000067D S_regpposixcc 0000045F @@ -647\,7 +647\,7 @@ S_try_yyparse 000000B3 S_uiv_2buf 0000004D S_unavailable 0000002E -S_unpack_rec 00001EFF +S_unpack_rec 00001EF6 S_unreferenced_to_tmp_stack 00000077 S_unshare_hek_or_pvn 0000015B S_unwind_handler_stack 00000009 @@ -925\,7 +925\,7 @@ _Perl_av_delete 00000158 _Perl_av_exists 00000195 _Perl_av_extend 00000097 -_Perl_av_extend_guts 0000019D +_Perl_av_extend_guts 000001A3 _Perl_av_fetch 00000102 _Perl_av_fill 000000EA _Perl_av_iter_p 00000013 @@ -954\,7 +954\,7 @@ _Perl_call_list 0000032D _Perl_call_method 0000005E _Perl_call_pv 00000024 -_Perl_call_sv 000003CD +_Perl_call_sv 000003D8 _Perl_caller_cx 000000D8 _Perl_calloc 0000001B _Perl_cast_iv 0000004C @@ -1073\,7 +1073\,7 @@ _Perl_do_gvgv_dump 00000203 _Perl_do_hv_dump 00000191 _Perl_do_join 000002C1 -_Perl_do_kv 00000259 +_Perl_do_kv 0000023C _Perl_do_magic_dump 000003C8 _Perl_do_ncmp 0000014D _Perl_do_op_dump 00000A0C @@ -1115\,7 +1115\,7 @@ _Perl_dump_vindent 00000039 _Perl_emulate_cop_io 000000B0 _Perl_eval_pv 00000118 -_Perl_eval_sv 0000028A +_Perl_eval_sv 00000293 _Perl_fbm_compile 000001FE _Perl_fbm_instr 00000321 _Perl_feature_is_enabled 0000005E @@ -1379\,7 +1379\,7 @@ _Perl_magic_getuvar 00000024 _Perl_magic_getvec 0000002B _Perl_magic_killbackrefs 0000001A -_Perl_magic_methcall 00000208 +_Perl_magic_methcall 0000020C _Perl_magic_nextpack 0000009C _Perl_magic_regdata_cnt 00000065 _Perl_magic_regdatum_get 000000B4 @@ -1597\,13 +1597\,13 @@ _Perl_pad_swipe 00000081 _Perl_pad_tidy 000001D0 _Perl_padlist_dup 00000226 -_Perl_padlist_store 00000069 +_Perl_padlist_store 0000006D _Perl_padname_dup 000000CD _Perl_padname_free 0000005E _Perl_padnamelist_dup 0000007B _Perl_padnamelist_fetch 00000016 _Perl_padnamelist_free 0000003E -_Perl_padnamelist_store 00000078 +_Perl_padnamelist_store 0000007B _Perl_parse_arithexpr 00000014 _Perl_parse_barestmt 0000002D _Perl_parse_block 0000002D @@ -1623\,14 +1623\,14 @@ _Perl_pmruntime 00000651 _Perl_pop_scope 0000001F _Perl_populate_isa 00000094 -_Perl_pp_aassign 0000081A +_Perl_pp_aassign 00000821 _Perl_pp_abs 00000107 _Perl_pp_accept 000001D2 _Perl_pp_add 00000283 _Perl_pp_aeach 000000C2 _Perl_pp_aelem 000002F2 _Perl_pp_aelemfast 000000E9 -_Perl_pp_akeys 00000123 +_Perl_pp_akeys 0000011D _Perl_pp_alarm 000000AC _Perl_pp_and 000000DB _Perl_pp_anoncode 0000005C @@ -1661\,7 +1661\,7 @@ _Perl_pp_cond_expr 000000DA _Perl_pp_const 00000046 _Perl_pp_continue 00000084 -_Perl_pp_coreargs 00000517 +_Perl_pp_coreargs 00000524 _Perl_pp_crypt 00000117 _Perl_pp_dbmopen 0000023B _Perl_pp_dbstate 0000029D @@ -1676\,7 +1676\,7 @@ _Perl_pp_entergiven 0000013C _Perl_pp_enteriter 00000441 _Perl_pp_enterloop 000000CE -_Perl_pp_entersub 00000835 +_Perl_pp_entersub 0000083D _Perl_pp_entertry 00000039 _Perl_pp_enterwhen 00000176 _Perl_pp_enterwrite 000000CB @@ -1689\,7 +1689\,7 @@ _Perl_pp_fileno 00000119 _Perl_pp_flip 00000251 _Perl_pp_flock 00000104 -_Perl_pp_flop 000005E0 +_Perl_pp_flop 000005E4 _Perl_pp_fork 00000082 _Perl_pp_formline 00000B30 _Perl_pp_ftis 0000019C @@ -1707\,10 +1707\,10 @@ _Perl_pp_getppid 00000014 _Perl_pp_getpriority 00000014 _Perl_pp_ghostent 00000255 -_Perl_pp_glob 000001E2 +_Perl_pp_glob 000001E3 _Perl_pp_gmtime 00000331 _Perl_pp_gnetent 00000024 -_Perl_pp_goto 000009D6 +_Perl_pp_goto 000009DA _Perl_pp_gprotoent 00000177 _Perl_pp_gpwent 00000024 _Perl_pp_grepstart 00000193 @@ -1741\,8 +1741\,8 @@ _Perl_pp_ioctl 0000028E _Perl_pp_iter 00000315 _Perl_pp_join 00000043 -_Perl_pp_kvaslice 000001DE -_Perl_pp_kvhslice 000001A3 +_Perl_pp_kvaslice 000001E2 +_Perl_pp_kvhslice 000001A7 _Perl_pp_last 000000BA _Perl_pp_lc 00000383 _Perl_pp_le 00000092 @@ -1766\,8 +1766\,8 @@ _Perl_pp_lvavref 0000004D _Perl_pp_lvref 000001A6 _Perl_pp_lvrefslice 00000173 -_Perl_pp_mapwhile 00000311 -_Perl_pp_match 0000059A +_Perl_pp_mapwhile 00000317 +_Perl_pp_match 000005A2 _Perl_pp_method 0000004A _Perl_pp_method_named 0000010F _Perl_pp_method_redir 00000124 @@ -1794\,10 +1794\,10 @@ _Perl_pp_or 000000E3 _Perl_pp_ord 00000107 _Perl_pp_pack 000000CD -_Perl_pp_padav 000001AE +_Perl_pp_padav 000001B2 _Perl_pp_padcv 0000003D _Perl_pp_padhv 00000160 -_Perl_pp_padrange 00000125 +_Perl_pp_padrange 0000012B _Perl_pp_padsv 00000075 _Perl_pp_pipe_op 00000192 _Perl_pp_pos 000000D8 @@ -1816\,7 +1816\,7 @@ _Perl_pp_range 000000D0 _Perl_pp_rcatline 0000001B _Perl_pp_readdir 00000142 -_Perl_pp_readline 000001EC +_Perl_pp_readline 000001F6 _Perl_pp_readlink 00000032 _Perl_pp_redo 000000CA _Perl_pp_ref 0000005D @@ -1825\,7 +1825\,7 @@ _Perl_pp_regcomp 000001EE _Perl_pp_regcreset 0000000E _Perl_pp_rename 000000C6 -_Perl_pp_repeat 0000043E +_Perl_pp_repeat 00000442 _Perl_pp_require 0000103A _Perl_pp_reset 00000095 _Perl_pp_return 0000012C @@ -1861\,15 +1861\,15 @@ _Perl_pp_sne 0000005C _Perl_pp_socket 000001C9 _Perl_pp_sockpair 000002B4 -_Perl_pp_sort 00000ADA -_Perl_pp_splice 000006BF -_Perl_pp_split 00000F42 +_Perl_pp_sort 00000ADE +_Perl_pp_splice 000006C3 +_Perl_pp_split 00000F63 _Perl_pp_sprintf 0000008E _Perl_pp_srand 00000107 _Perl_pp_srefgen 00000019 _Perl_pp_sselect 000002D1 _Perl_pp_ssockopt 00000266 -_Perl_pp_stat 00000570 +_Perl_pp_stat 00000572 _Perl_pp_stringify 0000003C _Perl_pp_stub 00000055 _Perl_pp_study 00000085 @@ -1885\,7 +1885\,7 @@ _Perl_pp_syswrite 000005E7 _Perl_pp_tell 0000013D _Perl_pp_telldir 000000DC -_Perl_pp_tie 0000047E +_Perl_pp_tie 0000048A _Perl_pp_tied 000000BB _Perl_pp_time 00000063 _Perl_pp_tms 000000E6 @@ -2053\,7 +2053\,7 @@ _Perl_sortsv 0000001B _Perl_sortsv_flags 0000003D _Perl_ss_dup 000004DA -_Perl_stack_grow 00000031 +_Perl_stack_grow 00000043 _Perl_start_glob 000001D5 _Perl_start_subparse 000000AE _Perl_str_to_version 000000B0 @@ -2218\,7 +2218\,7 @@ _Perl_sv_vcatpvf 0000002D _Perl_sv_vcatpvf_mg 0000003C _Perl_sv_vcatpvfn 0000002A -_Perl_sv_vcatpvfn_flags 000023AB +_Perl_sv_vcatpvfn_flags 000023AA _Perl_sv_vsetpvf 00000028 _Perl_sv_vsetpvf_mg 0000003C _Perl_sv_vsetpvfn 00000039 @@ -2232\,7 +2232\,7 @@ _Perl_sys_term 0000000F _Perl_taint_env 000002B3 _Perl_taint_proper 000000AC -_Perl_tied_method 00000244 +_Perl_tied_method 00000257 _Perl_tmps_grow_p 0000005A _Perl_to_uni_lower 00000115 _Perl_to_uni_lower_lc 0000001E @@ -2337\,7 +2337\,7 @@ _XS_re_is_regexp 00000062 _XS_re_regexp_pattern 000001A3 _XS_re_regname 00000173 -_XS_re_regnames 000001FF +_XS_re_regnames 00000203 _XS_re_regnames_count 000000B2 _XS_universal_version 0000028C _XS_utf8_decode 00000098 @@ -2635\,7 +2635\,7 @@ cmpindir 0000002C cmpindir_desc 00000032 compare(void const *\,void const *) 00000076 -const_av_xsub 00000117 +const_av_xsub 00000119 const_sv_xsub 0000005D core_xsub 00000026 create_command_line 0000026C

p5pRT commented 9 years ago

From @bulk88

pp_stat_after.PNG

p5pRT commented 9 years ago

From @bulk88

pp_stat_before.PNG

p5pRT commented 9 years ago

From @bulk88

On Sat Sep 26 06:08:10 2015\, bulk88 wrote:

There is one more strange thing about this branch but I need another hour to describe it so ill do it tomorrow.

I discovered HvUSEDKEYS() contains a getter-type function call\, and therefore HvUSEDKEYS suffers from multiple eval macro disease.

ext/B/B.xs | 10 ++++++++-- 1 files changed\, 8 insertions(+)\, 2 deletions(-)

Inline Patch

```diff diff --git a/ext/B/B.xs b/ext/B/B.xs index 5d15d80..eb21103 100644 --- a/ext/B/B.xs +++ b/ext/B/B.xs @@ -1370,7 +1370,9 @@ aux_list(o, cv) PAD *comppad = PadlistARRAY(padlist)[1]; #endif - EXTEND(SP, len); + /* len should never be big enough to truncate or wrap */ + assert(len <= SSize_t_MAX); + EXTEND(SP, (SSize_t)len); PUSHs(sv_2mortal(newSViv(actions))); while (!last) { @@ -2139,8 +2141,12 @@ HvARRAY(hv) PPCODE: if (HvUSEDKEYS(hv) > 0) { HE *he; + SSize_t extend_size; (void)hv_iterinit(hv); - EXTEND(sp, HvUSEDKEYS(hv) * 2); + /* 2*HvUSEDKEYS() should never be big enough to truncate or wrap */ + assert(HvUSEDKEYS(hv) <= (SSize_t_MAX >> 1)); + extend_size = (SSize_t)HvUSEDKEYS(hv) * 2; + EXTEND(sp, extend_size); while ((he = hv_iternext(hv))) { if (HeSVKEY(he)) { mPUSHs(HeSVKEY(he)); -------------------------------------------------------- ```

That accidentally (intentionally?) reduced 3 Perl_hv_placeholders_get fnc calls to 2 Perl_hv_placeholders_get fnc calls inside XS_B__HV_ARRAY. Also Perl_do_kv got a reduction in the number of Perl_hv_placeholders_get fnc calls.

lib/ExtUtils/typemap | 6 +++++- 1 files changed\, 5 insertions(+)\, 1 deletions(-)

Inline Patch

```diff diff --git a/lib/ExtUtils/typemap b/lib/ExtUtils/typemap index 5f61527..3efc1d5 100644 --- a/lib/ExtUtils/typemap +++ b/lib/ExtUtils/typemap @@ -378,7 +378,11 @@ T_PACKEDARRAY T_ARRAY { U32 ix_$var; - EXTEND(SP,size_$var); + SSize_t extend_size = + sizeof(size_$var) >= sizeof(SSize_t) && size_$var > SSize_t_MAX + ? -1 /* might wrap; -1 triggers a panic in EXTEND() */ + : (SSize_t)size_$var; + EXTEND(SP,extend_size); for (ix_$var = 0; ix_$var < size_$var; ix_$var++) { ST(ix_$var) = sv_newmortal(); DO_ARRAY_ELEM ------------------------------------------------- ```

The "sizeof(size_$var) >= sizeof(SSizet)" branch constant folded. The "size$var > SSize_t_MAX" branch didn't constant fold in XS_XS__Typemap_T_ARRAY and looks redundant to me. Reproducing the .c func below.

XS_EUPXS(XS_XS__Typemap_T_ARRAY); /* prototype to pass -Wmissing-prototypes */ XS_EUPXS(XS_XS__Typemap_T_ARRAY) { dVAR; dXSARGS; if (items \< 2) croak_xs_usage(cv\, "dummy\, array\, ..."); { int dummy = 0; intArray * array; #line 887 "Typemap.xs" U32 size_RETVAL; #line 1647 "Typemap.c" intArray * RETVAL;

U32 ix_array = 1; array = intArrayPtr(items -= 1); while (items--) { array[ix_array - 1] = (int)SvIV(ST(ix_array)) ; ix_array++; } /* this is the number of elements in the array */ ix_array -= 1 ; #line 889 "Typemap.xs" dummy += 0; /* Fix -Wall */ size_RETVAL = ix_array; RETVAL = array; #line 1664 "Typemap.c" { U32 ix_RETVAL; SSize_t extend_size = sizeof(size_RETVAL) >= sizeof(SSize_t) && size_RETVAL > SSize_t_MAX ? -1 /* might wrap; -1 triggers a panic in EXTEND() */ : (SSize_t)size_RETVAL; EXTEND(SP\,extend_size); for (ix_RETVAL = 0; ix_RETVAL \< size_RETVAL; ix_RETVAL++) { ST(ix_RETVAL) = sv_newmortal(); sv_setiv(ST(ix_RETVAL)\, (IV)RETVAL[ix_RETVAL]); } } #line 895 "Typemap.xs" Safefree(array); XSRETURN(size_RETVAL); #line 1680 "Typemap.c" } XSRETURN(1); }

disssembler for after davem branch inside XS_XS__Typemap_T_ARRAY\, VC 2003 -O1\, 32 IV\, 32 ptr\, pics also attached.

v16 = v10 - 1; v21 = v16; if ( (unsigned int)v16 > 0x7FFFFFFF ) { v17 = -1; LABEL_15: Perl_stack_grow(my_perl2\, stack_pointer\, stack_pointer\, v17); goto LABEL_16; } v17 = v16; if ( v16 \< 0 || (signed int)((char *)my_perl2->Istack_max - (_DWORD)stack_pointer) >> 2 \< v16 ) goto LABEL_15;

wouldn't "int n;" and then "(unsigned int)n > 0x7FFFFFFF" and "n \< 0" be identical? Should the " sizeof(size_$var) >= sizeof(SSize_t)" instead be "sizeof(size_$var) > sizeof(SSize_t)" and let the negative check in EXTEND macro take care of it if "sizeof(size_$var) == sizeof(SSize_t)"?

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 9 years ago

From @bulk88

XS_XS__Typemap_T_ARRAY_after.PNG

p5pRT commented 9 years ago

From @bulk88

XS_XS__Typemap_T_ARRAY_before.PNG

p5pRT commented 9 years ago

From @bulk88

--- C:\Documents and Settings\Owner\Desktop\predavemstack.txt +++ C:\Documents and Settings\Owner\Desktop\davemstack.txt @@ -1\,15 +1\,16 @@ -$L39468 0000000A -$L39840 0000008B -$L39888 000000EA -$L40463 0000005E -$L41904 00000298 -$L44027 000003BD -$L46285 000000E7 -$L64418 000000F7 -$L64807 0000029B -$L65188 0000010F -$L66079 000000D2 -$L66107 000000CC +$L39479_0 0000000A +$L39892 0000008B +$L39940 000000EA +$L41948 00000298 +$L44049 000003BD +$L62835 000000C1 +$L65843 000000F7 +$L66199 0000010F +$L67037 000000D2 +$L67090 000000CC +$L67118 000000CC +$L67242 00000120 +$L67325 0000015E BZ2_bzCompress(x\,x) 0000011A BZ2_bzCompressEnd(x) 00000065 BZ2_bzCompressInit(x\,x\,x\,x) 00000189 @@ -256\,7 +257\,7 @@ Process32Next(x\,x) 00000006 S_CvGV 00000019 S_F0convert 000000E3 -S_Internals_V 000000A9 +S_Internals_V 000000BA S_MgBYTEPOS 00000086 S_MgBYTEPOS_0 00000086 S_MgBYTEPOS_1 00000086 @@ -474\,7 +475\,7 @@ S_ithread_clear 00000058 S_ithread_create 00000320 S_ithread_free 000000FC -S_ithread_run 00000778 +S_ithread_run 00000781 S_ithread_to_SV 0000006B S_iv_shift 00000029 S_join_exact 00000B8D @@ -550\,7 +551\,7 @@ S_openn_setup 0000017A S_opmethod_stash 000002B7 S_outside_integer 00000079 -S_pack_rec 000025AF +S_pack_rec 000025B0 S_pad_alloc_name 0000006D S_pad_check_dup 00000183 S_pad_findlex 00000423 @@ -580\,7 +581\,7 @@ S_prune_chain_head 0000002D S_ptr_hash 0000003E S_push_slab 00000041 -S_pushav 000000DA +S_pushav 000000DE S_put_charclass_bitmap_innards 000001BA S_put_code_point 00000097 S_put_range 000002B9 @@ -627\,8 +628\,8 @@ S_reginclass_0 000002C4 S_reginsert 000000BC S_reginsert_0 00000477 -S_regmatch 00004CA0 -S_regmatch_0 00005D3C +S_regmatch 00004C34 +S_regmatch_0 00005CE0 S_regnode_guts 00000034 S_regnode_guts_0 0000016B S_regpatws 000000B0 @@ -753\,7 +754\,7 @@ S_try_yyparse 000000B3 S_uiv_2buf 0000004D S_unavailable 0000002E -S_unpack_rec 00001EFF +S_unpack_rec 00001EF6 S_unreferenced_to_tmp_stack 00000077 S_unshare_hek_or_pvn 0000015F S_unwind_handler_stack 00000009 @@ -858\,7 +859\,7 @@ XS_B__GV_is_empty 000000D0 XS_B__HE_HASH 000000CC XS_B__HE_VAL 000000DF -XS_B__HV_ARRAY 0000018B +XS_B__HV_ARRAY 00000174 XS_B__HV_FILL 000000CF XS_B__HV_RITER 000000D1 XS_B__IO_IsSTD 00000126 @@ -885\,7 +886\,7 @@ XS_B__RHE_HASH 0000009A XS_B__SV_REFCNT 000000E2 XS_B__SV_object_2svref 00000091 -XS_B__UNOP_AUX_aux_list 000002AC +XS_B__UNOP_AUX_aux_list 000002B0 XS_B__UNOP_AUX_string 000000FC XS_B_address 00000095 XS_B_amagic_generation 0000008F @@ -894\,6 +895\,7 @@ XS_B_formfeed 0000005F XS_B_hash 0000009D XS_B_main_root 0000005C +XS_B_minus_c 0000005E XS_B_opnumber 000000D7 XS_B_ppname 0000009E XS_B_sub_generation 000000A2 @@ -1016\,6 +1018\,7 @@ XS_Devel__PPPort_PUSHu 0000008F XS_Devel__PPPort_Perl_grok_bin 000000D8 XS_Devel__PPPort_Perl_grok_hex 000000D8 +XS_Devel__PPPort_Perl_grok_number 000000E7 XS_Devel__PPPort_Perl_grok_oct 000000D8 XS_Devel__PPPort_Perl_ppaddr_t 000000C3 XS_Devel__PPPort_Perl_sv_catpvf_mg 0000005B @@ -1225\,10 +1228\,10 @@ XS_File__Glob_bsd_glob_override 00000063 XS_File__Glob_csh_glob 0000005F XS_File__Spec__Unix__fn_canonpath 00000054 -XS_File__Spec__Unix__fn_catdir 000000AE +XS_File__Spec__Unix__fn_catdir 000000B2 XS_File__Spec__Unix__fn_catfile 000000FD XS_File__Spec__Unix_canonpath 00000066 -XS_File__Spec__Unix_catdir 0000015E +XS_File__Spec__Unix_catdir 00000162 XS_File__Spec__Unix_catfile 00000287 XS_Filter__Util__Call_filter_del 000000D4 XS_Filter__Util__Call_filter_read 0000010E @@ -1458\,11 +1461\,11 @@ XS_Time__Piece__strftime 000001E7 XS_Time__Piece__strptime 00000103 XS_Time__Piece__tzset 00000048 -XS_Unicode__Collate__decompHangul 0000010E +XS_Unicode__Collate__decompHangul 00000111 XS_Unicode__Collate__derivCE_8 00000127 XS_Unicode__Collate__derivCE_9 000001DE XS_Unicode__Collate__fetch_rest 00000087 -XS_Unicode__Collate__fetch_simple 000000FC +XS_Unicode__Collate__fetch_simple 00000100 XS_Unicode__Collate__getHexArray 00000148 XS_Unicode__Collate__ignorable_simple 000000D7 XS_Unicode__Collate__isIllegal 000000B1 @@ -1542\,7 +1545\,7 @@ XS_XS__APItest__Hash_rot13_hash 000000C9 XS_XS__APItest__Hash_store 000001B2 XS_XS__APItest__Hash_store_ent 00000179 -XS_XS__APItest__Hash_test_force_keys 00000168 +XS_XS__APItest__Hash_test_force_keys 0000016A XS_XS__APItest__Hash_test_hv_delayfree_ent 00000062 XS_XS__APItest__Hash_test_hv_free_ent 00000062 XS_XS__APItest__Hash_test_share_unshare_pvn 0000010C @@ -1560\,6 +1563\,7 @@ XS_XS__APItest__Magic_test_UVCHR_IS_INVARIANT 000000CE XS_XS__APItest__Magic_test_UVCHR_SKIP 0000014C XS_XS__APItest__Magic_test_get_vtbl 000004EC +XS_XS__APItest__Magic_test_isALNUM_LC_utf8 0000029B XS_XS__APItest__Magic_test_isALNUM_LC_uvchr 0000018F XS_XS__APItest__Magic_test_isALNUM_uni 000000F7 XS_XS__APItest__Magic_test_isALNUM_utf8 00000110 @@ -1576\,7 +1580\,6 @@ XS_XS__APItest__Magic_test_isALPHA_LC_uvchr 000001BD XS_XS__APItest__Magic_test_isALPHA_uni 000000F7 XS_XS__APItest__Magic_test_isALPHA_utf8 00000110 -XS_XS__APItest__Magic_test_isASCII_LC_utf8 000000CC XS_XS__APItest__Magic_test_isBLANK_A 000000E3 XS_XS__APItest__Magic_test_isBLANK_L1 000000DB XS_XS__APItest__Magic_test_isBLANK_LC 0000010B @@ -1665\,14 +1668\,12 @@ XS_XS__APItest__Magic_test_toFOLD_LC 00000157 XS_XS__APItest__Magic_test_toFOLD_uni 00000164 XS_XS__APItest__Magic_test_toFOLD_utf8 0000015B -XS_XS__APItest__Magic_test_toLOWER_L1 00000120 XS_XS__APItest__Magic_test_toLOWER_LC 0000013A XS_XS__APItest__Magic_test_toLOWER_uni 00000162 XS_XS__APItest__Magic_test_toLOWER_utf8 0000015B XS_XS__APItest__Magic_test_toTITLE 00000130 XS_XS__APItest__Magic_test_toTITLE_uni 00000162 XS_XS__APItest__Magic_test_toTITLE_utf8 0000015B -XS_XS__APItest__Magic_test_toUPPER_LC 0000015E XS_XS__APItest__Magic_test_toUPPER_uni 00000162 XS_XS__APItest__Magic_test_toUPPER_utf8 0000015B XS_XS__APItest__Overload_amagic_deref_call 000000BC @@ -1686\,6 +1687\,16 @@ XS_XS__APItest__TempLv_make_temp_mg_lv 00000137 XS_XS__APItest__XSUB_XS_APIVERSION_valid 00000062 XS_XS__APItest__XSUB_XS_VERSION_defined 00000062 +XS_XS__APItest__XSUB_xsreturn 00000122 +XS_XS__APItest__XSUB_xsreturn_empty 00000056 +XS_XS__APItest__XSUB_xsreturn_iv 00000086 +XS_XS__APItest__XSUB_xsreturn_no 00000071 +XS_XS__APItest__XSUB_xsreturn_nv 0000008D +XS_XS__APItest__XSUB_xsreturn_pv 00000089 +XS_XS__APItest__XSUB_xsreturn_pvn 00000080 +XS_XS__APItest__XSUB_xsreturn_undef 00000071 +XS_XS__APItest__XSUB_xsreturn_uv 00000086 +XS_XS__APItest__XSUB_xsreturn_yes 00000071 XS_XS__APItest__numeric_assertx 00000097 XS_XS__APItest__numeric_grok_atoUV 0000018B XS_XS__APItest__numeric_grok_number 0000011F @@ -1722\,7 +1733\,6 @@ XS_XS__APItest_gv_fetchmeth_type 0000029B XS_XS__APItest_gv_fetchmethod_flags_type 00000271 XS_XS__APItest_gv_init_type 00000283 -XS_XS__APItest_have_long_double 000000C1 XS_XS__APItest_lexical_import 000001E7 XS_XS__APItest_lv_temp_object 000000BA XS_XS__APItest_mpushi 000000E6 @@ -1730\,7 +1740\,7 @@ XS_XS__APItest_mpushp 000000D8 XS_XS__APItest_mpushu 000000E6 XS_XS__APItest_multicall_each 00000575 -XS_XS__APItest_multicall_return 0000062A +XS_XS__APItest_multicall_return 0000062C XS_XS__APItest_mxpushi 00000133 XS_XS__APItest_mxpushn 00000148 XS_XS__APItest_mxpushp 00000124 @@ -1773\,6 +1783\,7 @@ XS_XS__APItest_take_cvref 000000D7 XS_XS__APItest_take_hvref 000000C5 XS_XS__APItest_take_svref 000000BE +XS_XS__APItest_test_EXTEND 0000027B XS_XS__APItest_test_alloccopstash 000000B1 XS_XS__APItest_test_cophh 000015DA XS_XS__APItest_test_coplabel 00000112 @@ -1802\,7 +1813\,7 @@ XS_XS__APItest_xop_register 000000AC XS_XS__APItest_xs_cmp 00000192 XS_XS__APItest_xs_cmp_undef 0000008A -XS_XS__Typemap_T_ARRAY 00000121 +XS_XS__Typemap_T_ARRAY 00000134 XS_XS__Typemap_T_AVREF 00000090 XS_XS__Typemap_T_AVREF_REFCOUNT_FIXED 00000097 XS_XS__Typemap_T_BOOL 000000E0 @@ -2229\,7 +2240\,7 @@ _Perl_av_delete 0000016A _Perl_av_exists 0000018D _Perl_av_extend 00000098 -_Perl_av_extend_guts 0000019A +_Perl_av_extend_guts 000001B2 _Perl_av_fetch 00000103 _Perl_av_fill 000000E8 _Perl_av_iter_p 00000013 @@ -2258\,7 +2269\,7 @@ _Perl_call_list 0000031A _Perl_call_method 0000005E _Perl_call_pv 00000024 -_Perl_call_sv 000003CF +_Perl_call_sv 000003DA _Perl_caller_cx 000000D8 _Perl_calloc 0000001B _Perl_cando 00000014 @@ -2378\,7 +2389\,7 @@ _Perl_do_gvgv_dump 00000203 _Perl_do_hv_dump 00000191 _Perl_do_join 000002C1 -_Perl_do_kv 00000281 +_Perl_do_kv 0000025D _Perl_do_magic_dump 000003C8 _Perl_do_ncmp 00000153 _Perl_do_op_dump 00000A0C @@ -2420\,7 +2431\,7 @@ _Perl_dump_vindent 00000039 _Perl_emulate_cop_io 000000B0 _Perl_eval_pv 00000118 -_Perl_eval_sv 0000028A +_Perl_eval_sv 00000293 _Perl_fbm_compile 000001FE _Perl_fbm_instr 00000322 _Perl_feature_is_enabled 0000005E @@ -2684\,7 +2695\,7 @@ _Perl_magic_getuvar 00000024 _Perl_magic_getvec 0000002C _Perl_magic_killbackrefs 0000001A -_Perl_magic_methcall 00000208 +_Perl_magic_methcall 0000020C _Perl_magic_nextpack 000000A9 _Perl_magic_regdata_cnt 00000065 _Perl_magic_regdatum_get 000000B4 @@ -2930\,14 +2941\,14 @@ _Perl_pmruntime 0000062E _Perl_pop_scope 0000001F _Perl_populate_isa 00000094 -_Perl_pp_aassign 00000853 +_Perl_pp_aassign 0000085A _Perl_pp_abs 00000107 _Perl_pp_accept 000001D4 _Perl_pp_add 00000283 _Perl_pp_aeach 000000D5 _Perl_pp_aelem 000002DF _Perl_pp_aelemfast 000000E9 -_Perl_pp_akeys 00000136 +_Perl_pp_akeys 0000012D _Perl_pp_alarm 000000AC _Perl_pp_and 000000DB _Perl_pp_anoncode 0000005C @@ -2968\,7 +2979\,7 @@ _Perl_pp_cond_expr 000000DA _Perl_pp_const 00000046 _Perl_pp_continue 0000009C -_Perl_pp_coreargs 0000050F +_Perl_pp_coreargs 0000051C _Perl_pp_crypt 00000117 _Perl_pp_dbmopen 00000235 _Perl_pp_dbstate 0000029D @@ -2983\,7 +2994\,7 @@ _Perl_pp_entergiven 0000013C _Perl_pp_enteriter 00000441 _Perl_pp_enterloop 000000CE -_Perl_pp_entersub 0000084F +_Perl_pp_entersub 00000852 _Perl_pp_entertry 00000039 _Perl_pp_enterwhen 00000176 _Perl_pp_enterwrite 000000CB @@ -2996\,7 +3007\,7 @@ _Perl_pp_fileno 00000119 _Perl_pp_flip 00000251 _Perl_pp_flock 00000106 -_Perl_pp_flop 000005E0 +_Perl_pp_flop 000005E4 _Perl_pp_fork 00000082 _Perl_pp_formline 00000AF8 _Perl_pp_ftis 0000019C @@ -3014\,10 +3025\,10 @@ _Perl_pp_getppid 00000014 _Perl_pp_getpriority 00000014 _Perl_pp_ghostent 00000255 -_Perl_pp_glob 000001F1 +_Perl_pp_glob 000001F5 _Perl_pp_gmtime 00000325 _Perl_pp_gnetent 00000024 -_Perl_pp_goto 000009EB +_Perl_pp_goto 000009EE _Perl_pp_gprotoent 00000177 _Perl_pp_gpwent 00000024 _Perl_pp_grepstart 00000193 @@ -3048\,8 +3059\,8 @@ _Perl_pp_ioctl 0000028F _Perl_pp_iter 00000314 _Perl_pp_join 00000043 -_Perl_pp_kvaslice 000001C4 -_Perl_pp_kvhslice 000001A3 +_Perl_pp_kvaslice 000001C8 +_Perl_pp_kvhslice 000001A7 _Perl_pp_last 000000BA _Perl_pp_lc 00000385 _Perl_pp_le 00000094 @@ -3073\,8 +3084\,8 @@ _Perl_pp_lvavref 0000004D _Perl_pp_lvref 000001A0 _Perl_pp_lvrefslice 0000016D -_Perl_pp_mapwhile 00000311 -_Perl_pp_match 000005B6 +_Perl_pp_mapwhile 00000317 +_Perl_pp_match 000005BA _Perl_pp_method 0000004A _Perl_pp_method_named 0000010F _Perl_pp_method_redir 00000124 @@ -3101\,10 +3112\,10 @@ _Perl_pp_or 000000E3 _Perl_pp_ord 00000107 _Perl_pp_pack 000000CD -_Perl_pp_padav 000001BD +_Perl_pp_padav 000001B3 _Perl_pp_padcv 0000003D _Perl_pp_padhv 00000159 -_Perl_pp_padrange 00000125 +_Perl_pp_padrange 0000012B _Perl_pp_padsv 00000075 _Perl_pp_pipe_op 00000192 _Perl_pp_pos 000000D8 @@ -3123\,7 +3134\,7 @@ _Perl_pp_range 000000D0 _Perl_pp_rcatline 0000001E _Perl_pp_readdir 00000142 -_Perl_pp_readline 000001ED +_Perl_pp_readline 000001F7 _Perl_pp_readlink 00000032 _Perl_pp_redo 000000CA _Perl_pp_ref 0000005F @@ -3132\,7 +3143\,7 @@ _Perl_pp_regcomp 000001EE _Perl_pp_regcreset 0000000E _Perl_pp_rename 000000C6 -_Perl_pp_repeat 00000437 +_Perl_pp_repeat 0000043B _Perl_pp_require 00000F2A _Perl_pp_reset 00000095 _Perl_pp_return 0000012C @@ -3168\,15 +3179\,15 @@ _Perl_pp_sne 0000005C _Perl_pp_socket 000001C9 _Perl_pp_sockpair 000002B4 -_Perl_pp_sort 00000B02 -_Perl_pp_splice 000006BF -_Perl_pp_split 00000F3F +_Perl_pp_sort 00000B06 +_Perl_pp_splice 000006C3 +_Perl_pp_split 00000F60 _Perl_pp_sprintf 0000009C _Perl_pp_srand 00000107 _Perl_pp_srefgen 00000019 _Perl_pp_sselect 000002D1 _Perl_pp_ssockopt 00000268 -_Perl_pp_stat 00000569 +_Perl_pp_stat 0000056B _Perl_pp_stringify 0000003C _Perl_pp_stub 00000055 _Perl_pp_study 00000085 @@ -3192\,7 +3203\,7 @@ _Perl_pp_syswrite 000005EB _Perl_pp_tell 00000145 _Perl_pp_telldir 000000DC -_Perl_pp_tie 0000047E +_Perl_pp_tie 0000048A _Perl_pp_tied 000000BC _Perl_pp_time 00000063 _Perl_pp_tms 000000E6 @@ -3369\,7 +3380\,7 @@ _Perl_sortsv 0000001B _Perl_sortsv_flags 0000003D _Perl_ss_dup 000004DA -_Perl_stack_grow 00000031 +_Perl_stack_grow 00000043 _Perl_start_glob 000001DB _Perl_start_subparse 000000AE _Perl_str_to_version 000000B0 @@ -3534\,7 +3545\,7 @@ _Perl_sv_vcatpvf 0000002D _Perl_sv_vcatpvf_mg 0000003C _Perl_sv_vcatpvfn 0000002A -_Perl_sv_vcatpvfn_flags 00002238 +_Perl_sv_vcatpvfn_flags 0000223C _Perl_sv_vsetpvf 0000003F _Perl_sv_vsetpvf_mg 0000003C _Perl_sv_vsetpvfn 00000039 @@ -3548\,7 +3559\,7 @@ _Perl_sys_term 0000000F _Perl_taint_env 000002C2 _Perl_taint_proper 000000AC -_Perl_tied_method 00000257 +_Perl_tied_method 00000267 _Perl_tmps_grow_p 0000005A _Perl_to_uni_lower 00000115 _Perl_to_uni_lower_lc 0000001E @@ -3658\,7 +3669\,7 @@ _XS_re_is_regexp 00000062 _XS_re_regexp_pattern 000001D5 _XS_re_regname 00000173 -_XS_re_regnames 0000020E +_XS_re_regnames 0000020A _XS_re_regnames_count 000000B2 _XS_release_anotherstructPtrPtr 00000025 _XS_universal_version 00000289 @@ -3801\,7 +3812\,7 @@ _boot_Time__Piece 000000D5 _boot_Unicode__Collate 000001A3 _boot_Win32 0000040B -_boot_XS__APItest 000024A2 +_boot_XS__APItest 000025AA _boot_XS__Typemap 000003EC _boot_arybase 000001FB _boot_attributes 00000088 @@ -4224\,7 +4235\,7 @@ compare 00000014 compare(void const *\,void const *) 00000076 compress_block 00000399 -const_av_xsub 00000117 +const_av_xsub 00000119 const_sv_xsub 0000005D constant 00000224 constant_0 000000FF @@ -4276\,7 +4287\,7 @@ do_spawnvp_handles 000002DE do_store 00000134 doencodes 000000E1 -doglob 000000C0 +doglob 000000C6 doglob_iter_wrapper 000000CE dup_md5_ctx 0000002A dynprep 000001BD @@ -4346\,7 +4357\,7 @@ is_common 00000173 is_dollar_bracket 00000056 isempty_RL 00000015 -iterate 000002C2 +iterate 000002C4 ithread_mg_dup 00000022 ithread_mg_free 00000022 ithread_mg_get 00000017

p5pRT commented 9 years ago

From @iabyn

On Mon\, Sep 28\, 2015 at 08:19:09PM -0700\, bulk88 via RT wrote:

That accidentally (intentionally?) reduced 3 Perl_hv_placeholders_get fnc calls to 2 Perl_hv_placeholders_get fnc calls inside XS_B__HV_ARRAY. Also Perl_do_kv got a reduction in the number of Perl_hv_placeholders_get fnc calls.

That was intentional.

Should the " sizeof(size_$var) >= sizeof(SSize_t)" instead be "sizeof(size_$var) > sizeof(SSize_t)" and let the negative check in EXTEND macro take care of it if "sizeof(size_$var) == sizeof(SSize_t)"?

Yes\, that was a typo. It was supposed to be >.

-- Lear: Dost thou call me fool\, boy? Fool: All thy other titles thou hast given away; that thou wast born with.

p5pRT commented 9 years ago

From @iabyn

On Wed\, Sep 30\, 2015 at 11:07:27AM +0100\, Dave Mitchell wrote:

On Mon\, Sep 28\, 2015 at 08:19:09PM -0700\, bulk88 via RT wrote:

That accidentally (intentionally?) reduced 3 Perl_hv_placeholders_get fnc calls to 2 Perl_hv_placeholders_get fnc calls inside XS_B__HV_ARRAY. Also Perl_do_kv got a reduction in the number of Perl_hv_placeholders_get fnc calls.

That was intentional.

Should the " sizeof(size_$var) >= sizeof(SSize_t)" instead be "sizeof(size_$var) > sizeof(SSize_t)" and let the negative check in EXTEND macro take care of it if "sizeof(size_$var) == sizeof(SSize_t)"?

Yes\, that was a typo. It was supposed to be >.

Now fixed up and merged into blead with e1e5757.

-- More than any other time in history\, mankind faces a crossroads. One path leads to despair and utter hopelessness. The other\, to total extinction. Let us pray we have the wisdom to choose correctly. -- Woody Allen

p5pRT commented 9 years ago

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

p5pRT commented 8 years ago

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 commented 8 years ago

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

Perl / perl5