Skip Menu |
Report information

CC: perl5-porters [...] perl.org
Subject: 'x' operator on list causes segfault with possible stack corruption
Download (untitled) / with headers
text/plain 6.6k
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 .
Date: Wed, 2 Sep 2015 15:55:29 +0100
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 2.2k
On Fri, Aug 28, 2015 at 09:12:12PM -0700, Dan Collins wrote: Show quoted text
> 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.
RT-Send-CC: davem [...] iabyn.com, perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 10.6k
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.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.5k
On Wed Sep 02 07:56:01 2015, davem wrote: Show quoted text
> 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
From: Dave Mitchell <davem [...] iabyn.com>
Date: Sun, 6 Sep 2015 11:21:46 +0100
To: Tony Cook via RT <perlbug-followup [...] perl.org>
CC: perl5-porters [...] perl.org
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
Download (untitled) / with headers
text/plain 740b
On Wed, Sep 02, 2015 at 10:17:03PM -0700, Tony Cook via RT wrote: Show quoted text
> 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).
To: Tony Cook via RT <perlbug-followup [...] perl.org>
CC: perl5-porters [...] perl.org
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
From: Dave Mitchell <davem [...] iabyn.com>
Date: Mon, 7 Sep 2015 15:31:31 +0100
Download (untitled) / with headers
text/plain 5.9k
On Wed, Sep 02, 2015 at 10:17:03PM -0700, Tony Cook via RT wrote: Show quoted text
> For the case where I see the crash:
[snip] Show quoted text
> 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 < p || 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). 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<TARG>. See also C<L</XPUSHu>>, C<L</mPUSHu>> and C<L</PUSHu>>. #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 < p || 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 < p || 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.
Date: Mon, 07 Sep 2015 14:29:10 -0500
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
CC: Tony Cook via RT <perlbug-followup [...] perl.org>, perl5-porters [...] perl.org
To: Dave Mitchell <davem [...] iabyn.com>
From: Douglas Bell <preaction [...] me.com>
Download (untitled) / with headers
text/plain 949b

Show quoted text
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?



From: Dave Mitchell <davem [...] iabyn.com>
Date: Tue, 8 Sep 2015 08:23:01 +0100
CC: Tony Cook via RT <perlbug-followup [...] perl.org>, perl5-porters [...] perl.org
To: Douglas Bell <preaction [...] me.com>
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
Download (untitled) / with headers
text/plain 466b
On Mon, Sep 07, 2015 at 02:29:10PM -0500, Douglas Bell wrote: Show quoted text
> 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
RT-Send-CC: perl5-porters [...] perl.org
On Mon Sep 07 07:32:01 2015, davem wrote: Show quoted text
> 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. Show quoted text
> and the second > > SV ** const newp = p + (n); > if (UNLIKELY(newp < p || 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. Show quoted text
> 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
Date: Mon, 14 Sep 2015 17:35:18 +0100
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
To: Tony Cook via RT <perlbug-followup [...] perl.org>
CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.4k
On Thu, Sep 10, 2015 at 06:21:49PM -0700, Tony Cook via RT wrote: Show quoted text
> > 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.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 3.1k
On Mon Sep 07 07:32:01 2015, davem wrote: Show quoted text
> 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 < p || 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). >
Show quoted text
> # 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 < p || 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
From: Dave Mitchell <davem [...] iabyn.com>
Date: Mon, 21 Sep 2015 15:09:34 +0100
To: bulk88 via RT <perlbug-followup [...] perl.org>
CC: perl5-porters [...] perl.org
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
Download (untitled) / with headers
text/plain 2.9k
On Sun, Sep 20, 2015 at 12:12:37PM -0700, bulk88 via RT wrote: Show quoted text
> 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. Show quoted text
> 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. Show quoted text
> 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.
Date: Mon, 21 Sep 2015 15:29:03 +0100
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
From: Zefram <zefram [...] fysh.org>
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 340b
Dave Mitchell wrote: Show quoted text
>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
To: Zefram <zefram [...] fysh.org>
CC: perl5-porters [...] perl.org
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
From: Dave Mitchell <davem [...] iabyn.com>
Date: Mon, 21 Sep 2015 16:44:56 +0100
Download (untitled) / with headers
text/plain 1.1k
On Mon, Sep 21, 2015 at 03:29:03PM +0100, Zefram wrote: Show quoted text
> 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
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
Date: Mon, 21 Sep 2015 17:12:23 +0100
From: Zefram <zefram [...] fysh.org>
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.5k
Dave Mitchell wrote: Show quoted text
>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. Show quoted text
> 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
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
To: Zefram <zefram [...] fysh.org>
CC: perl5-porters [...] perl.org
Date: Tue, 22 Sep 2015 10:03:22 +0100
From: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 769b
On Mon, Sep 21, 2015 at 05:12:23PM +0100, Zefram wrote: Show quoted text
> __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.
From: Zefram <zefram [...] fysh.org>
To: perl5-porters [...] perl.org
Date: Tue, 22 Sep 2015 12:45:53 +0100
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
Download (untitled) / with headers
text/plain 682b
Dave Mitchell wrote: Show quoted text
>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
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
To: Zefram <zefram [...] fysh.org>
CC: perl5-porters [...] perl.org
Date: Tue, 22 Sep 2015 22:08:09 +0100
From: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 1.5k
On Tue, Sep 22, 2015 at 12:45:53PM +0100, Zefram wrote: Show quoted text
> 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.
Date: Tue, 22 Sep 2015 18:46:19 -0600
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
CC: perl5-porters [...] perl.org
To: Dave Mitchell <davem [...] iabyn.com>, Zefram <zefram [...] fysh.org>
From: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 926b
On 09/22/2015 03:08 PM, Dave Mitchell wrote: Show quoted text
> 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
Date: Wed, 23 Sep 2015 10:55:53 +0100
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
From: Zefram <zefram [...] fysh.org>
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 775b
Dave Mitchell wrote: Show quoted text
>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. Show quoted text
>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. Show quoted text
> 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
Date: Wed, 23 Sep 2015 14:22:24 +0100
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
To: Zefram <zefram [...] fysh.org>
CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.7k
On Wed, Sep 23, 2015 at 10:55:53AM +0100, Zefram wrote: Show quoted text
> > 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
From: Zefram <zefram [...] fysh.org>
To: perl5-porters [...] perl.org
Date: Wed, 23 Sep 2015 15:06:56 +0100
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
Download (untitled) / with headers
text/plain 404b
Dave Mitchell wrote: Show quoted text
>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
From: Dave Mitchell <davem [...] iabyn.com>
Date: Thu, 24 Sep 2015 17:51:11 +0100
To: perl5-porters [...] perl.org
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
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.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 5.6k
On Thu Sep 24 09:52:00 2015, davem wrote: Show quoted text
> > 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(-) 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
Subject: davemstackfncdelta.txt

Message body is not shown because it is too large.

Date: Fri, 25 Sep 2015 11:33:24 +0100
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
To: bulk88 via RT <perlbug-followup [...] perl.org>
CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 2.7k
On Thu, Sep 24, 2015 at 04:10:33PM -0700, bulk88 via RT wrote: Show quoted text
> > 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] Show quoted text
> The change is probably caused by this hunk.
[snip] Show quoted text
> # 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? Show quoted text
> 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. Show quoted text
> 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".
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 2.5k
On Fri Sep 25 03:33:53 2015, davem wrote: Show quoted text
> 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 155b
On Sat Sep 26 06:08:10 2015, bulk88 wrote: Show quoted text
> pics attached of the extra jump. >
Attachments didnt make it, reposting. -- bulk88 ~ bulk88 at hotmail.com
Subject: davemstackfncdelta.txt

Message body is not shown because it is too large.

Subject: pp_stat_after.PNG
Download pp_stat_after.PNG
image/png 15.5k
pp_stat_after.PNG
Subject: pp_stat_before.PNG
Download pp_stat_before.PNG
image/png 12.9k
pp_stat_before.PNG
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 4.9k
On Sat Sep 26 06:08:10 2015, bulk88 wrote: Show quoted text
> 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(-) 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(-) 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(SSize_t)" 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
Subject: XS_XS__Typemap_T_ARRAY_after.PNG
XS_XS__Typemap_T_ARRAY_after.PNG
Subject: XS_XS__Typemap_T_ARRAY_before.PNG
XS_XS__Typemap_T_ARRAY_before.PNG
Subject: davemstackfncdelta-staticperl.txt

Message body is not shown because it is too large.

Date: Wed, 30 Sep 2015 11:07:27 +0100
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
CC: perl5-porters [...] perl.org
To: bulk88 via RT <perlbug-followup [...] perl.org>
Download (untitled) / with headers
text/plain 696b
On Mon, Sep 28, 2015 at 08:19:09PM -0700, bulk88 via RT wrote: Show quoted text
> 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. Show quoted text
> 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.
CC: perl5-porters [...] perl.org
To: bulk88 via RT <perlbug-followup [...] perl.org>
Subject: Re: [perl #125937] 'x' operator on list causes segfault with possible stack corruption
From: Dave Mitchell <davem [...] iabyn.com>
Date: Fri, 2 Oct 2015 11:45:12 +0100
Download (untitled) / with headers
text/plain 943b
On Wed, Sep 30, 2015 at 11:07:27AM +0100, Dave Mitchell wrote: Show quoted text
> 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
Download (untitled) / with headers
text/plain 252b
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


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org