'x' operator on list causes segfault with possible stack corruption #14880
Comments
From @dcollinsnGreetings 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 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 **BISECT** b3b27d0 is the first bad commit repeat op: avoid integer overflows For the list variant of the x operator, the overflow detection code There are two places where an overflow could occur: calculating how The overflow detection was generally a mess; checking for overflow 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; :100644 100644 f7957256e80945e4fec2f0fb1947c492c4c19cc2 ec0f9d31ec203d942bd8570c97f7f06f6294f403 M pp.c **PERL -V** dcollins@nagios:~$ /usr/local/perl-afl/bin/perl -V Characteristics of this binary (from libperl): |
From @iabynOn Fri, Aug 28, 2015 at 09:12:12PM -0700, Dan Collins wrote:
Unfortunately I can't reproduce this. It's made harder by the fact that $ perl -le'print 0+{}' This makes the count highly dependent on OS etc. Also, valgrind tends to valgrind perl -le'print 0+{}' 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, # set virtual memory limit to 5Gb $ p -e'f$0(d..M)x28_459_935' $ p -e'f$0(d..M)x28_459_936' I don't see any different transitional behaviour near the ulimit Are you able to reproduce it using a fixed ulimit and a literal count? Also, what is your version of glibc? Thanks. -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @dcollinsnI can reproduce this: dcollins@nagios:~/perl$ ./perl -e 'f$0(d..M)x{}' 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' As you can see, things get all sorts of pear-shaped after that batch of illegal accesses. |
From @tonycozOn Wed Sep 02 07:56:01 2015, davem wrote:
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) { 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 */ produces a reasonable: [tony@localhost perl]$ ./perl -e 'f$0(d..M)x{}' Tony |
From @iabynOn Wed, Sep 02, 2015 at 10:17:03PM -0700, Tony Cook via RT wrote:
Yeah, I spotted that a bit later. I've been working on a general fix but a -- |
From @iabynOn Wed, Sep 02, 2015 at 10:17:03PM -0700, Tony Cook via RT wrote:
I think the deeper bug is that MEXTEND() (and EXTEND(), and various other So I think that pp_repeat() is wrong for passing an unsigned value to Currently the core of EXTEND/MEXTEND look like: if (UNLIKELY(PL_stack_max - p < (SSize_t)(n))) I think the two things wrong with that are that the casts just quietly I can see two approaches to fixing it (and similar macros). Both if (UNLIKELY((n) < 0 || p + (n) > PL_stack_max)) { and the second SV ** const newp = p + (n); They are both more or less equivalent; the major difference is that I'd be interested in opinions on which approach is best (or if there's a In either case, this change just means that stack_grow() is now called on Inline Patchdiff --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. |
From preaction@me.com
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> |
From @iabynOn Mon, Sep 07, 2015 at 02:29:10PM -0500, Douglas Bell wrote:
Probably. I'll review your stuff while I'm at it. -- |
From @tonycozOn Mon Sep 07 07:32:01 2015, davem wrote:
This seems safe, though the warnings will be an issue.
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.
Makes sense to me. Tony |
From @iabynOn Thu, Sep 10, 2015 at 06:21:49PM -0700, Tony Cook via RT wrote:
Thinking about it further, that's not safe either. For example if p and n I'm tempted to go back to the original scheme, but without the cast; 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 downside of it is that the compiler will complain about Does anyone think that warning would be a bad thing? -- |
From @bulk88On Mon Sep 07 07:32:01 2015, davem 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). 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. -- |
From @iabynOn Sun, Sep 20, 2015 at 12:12:37PM -0700, bulk88 via RT wrote:
The vast bulk of those calls to EXTEND() have a literal constant N, in
But the whole point is that without the extra (n < 0) test,
But it may well error out by stomping over the end of the stack which Anyway, my current code looks like this and seems to working well on both #define _EXTEND_SAFE_N(n) \ # define _EXTEND_NEEDS_GROW(p,n) ( (n) < 0 || PL_stack_max - p < (n)) # define EXTEND(p,n) STMT_START { \ -- |
From zefram@fysh.orgDave Mitchell wrote:
You could use gcc's __builtin_constant_p() to finesse this, only including -zefram |
From @iabynOn Mon, Sep 21, 2015 at 03:29:03PM +0100, Zefram wrote:
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 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) ... -- |
From zefram@fysh.orgDave Mitchell wrote:
But you have a choice as to whether to do that test inline or in the
There are three essentially different places where the logic can be __builtin_constant_p() lets you split those cases, making the manual I haven't reached an opinion myself about whether that's a good approach -zefram |
From @iabynOn Mon, Sep 21, 2015 at 05:12:23PM +0100, Zefram wrote:
But if we avoid bloating the call site by putting the n<0 test in the -- |
From zefram@fysh.orgDave Mitchell wrote:
Oh, I see what you're getting at. I believe that for the purpose of -zefram |
From @iabynOn Tue, Sep 22, 2015 at 12:45:53PM +0100, Zefram wrote:
But casting n to an unsigned type might truncate n if we choose too small I suppose we could cast n to a UV on the grounds that UV is guaranteed to Also, I can't currently think of any reformulation of max - p < n into Frankly C's int rules do my head in every time I go near them. -- |
From @khwilliamsonOn 09/22/2015 03:08 PM, Dave Mitchell wrote:
I tried to solve this kind of issue in handy.h, and there have been no /* Specify the widest unsigned type on the platform. Use U64TYPE |
From zefram@fysh.orgDave Mitchell wrote:
True. There is (u)intmax_t, but that might be a portability concern.
size_t would be the natural choice. But it is possible to have a larger
p can't be > max, unless the stack is already blown. Going further -zefram |
From @iabynOn Wed, Sep 23, 2015 at 10:55:53AM +0100, Zefram wrote:
Looking further, The way the EXTEND/MEXTEND interface is designed, it is For example in pp_leavesub(), in the scalar context when there are no args 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; -- |
From zefram@fysh.orgDave Mitchell wrote:
Eep. *That* invokes undefined behaviour. But as long as we have it, -zefram |
From @iabynI'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 It contains the following commits, along with Doug's two XSRETURN() commit c319c24 fix up EXTEND() callers commit 8820966 make EXTEND() and stack_grow() safe(r) commit f25d48a fix some 32/64-bit compiler warnings -- |
From @bulk88On Thu Sep 24 09:52:00 2015, davem wrote:
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 * before at e120c24 disassebled from perl523.dll after VC -O1 optimization. padname *__cdecl Perl_newPADNAMEpvn(const char *s, unsigned int len) v3 = Perl_safesyscalloc(len + 31, 1u); after at 7a36e61 padname *__cdecl Perl_newPADNAMEpvn(const char *s, unsigned int len) if ( len + 31 <= 0xFFFFFFFF ) 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 The change is probably caused by this hunk. * fix some 32/64-bit compiler warnings f25d48a Inline Patchdiff --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))))
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 -- |
From @iabynOn Thu, Sep 24, 2015 at 04:10:33PM -0700, bulk88 via RT wrote:
Thanks for spotting this. The '<=' should have been '<'. It turns out I've now pushed extend2 for smoking with this change (plus a fix for
My original branch tests for negativity in Perl_av_extend_guts() too, so
Personally I've already spent more time on EXTEND() than I wanted too; -- |
From @bulk88On Fri Sep 25 03:33:53 2015, davem wrote:
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: ef058b3 * POD fix in the documentation for SvTHINKFIRST after commit on extend2 branch is SHA-1: b32395d * 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 ) to if ( v49 < 0 || (signed int)((char *)my_perl->Istack_max - (_DWORD)v2) >> 2 < 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. -- |
From @bulk88On Sat Sep 26 06:08:10 2015, bulk88 wrote:
Attachments didnt make it, reposting. -- |
From @bulk88--- C:\Documents and Settings\Owner\Desktop\predavemstack.txt |
From @bulk88 |
From @bulk88On Sat Sep 26 06:08:10 2015, bulk88 wrote:
I discovered HvUSEDKEYS() contains a getter-type function call, and therefore HvUSEDKEYS suffers from multiple eval macro disease. ext/B/B.xs | 10 ++++++++-- Inline Patchdiff --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));
--------------------------------------------------------
lib/ExtUtils/typemap | 6 +++++- Inline Patchdiff --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
-------------------------------------------------
XS_EUPXS(XS_XS__Typemap_T_ARRAY); /* prototype to pass -Wmissing-prototypes */ U32 ix_array = 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; 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)"? -- |
From @bulk88--- C:\Documents and Settings\Owner\Desktop\predavemstack.txt |
From @iabynOn Mon, Sep 28, 2015 at 08:19:09PM -0700, bulk88 via RT wrote:
That was intentional.
Yes, that was a typo. It was supposed to be >. -- |
From @iabynOn Wed, Sep 30, 2015 at 11:07:27AM +0100, Dave Mitchell wrote:
Now fixed up and merged into blead with e1e5757. -- |
|
@iabyn - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for submitting this report. You have helped make Perl better. Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0 |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#125937 (status was 'resolved')
Searchable as RT125937$
The text was updated successfully, but these errors were encountered: