diff options
author | David Mitchell <davem@iabyn.com> | 2015-09-07 15:00:32 +0100 |
---|---|---|
committer | David Mitchell <davem@iabyn.com> | 2015-10-02 11:18:17 +0100 |
commit | 6768377c79109b7124f0c8a4e3677982689d9f49 (patch) | |
tree | 45dd3a0e222133612ae03b1397445d19fb1f2c0e /MANIFEST | |
parent | 73e8ff0004522621dfb42f01966853b51d5522a6 (diff) | |
download | perl-6768377c79109b7124f0c8a4e3677982689d9f49.tar.gz |
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));
Diffstat (limited to 'MANIFEST')
-rw-r--r-- | MANIFEST | 1 |
1 files changed, 1 insertions, 0 deletions
@@ -3933,6 +3933,7 @@ ext/XS-APItest/t/customop.t XS::APItest: tests for custom ops ext/XS-APItest/t/cv_name.t test cv_name ext/XS-APItest/t/eval-filter.t Simple source filter/eval test ext/XS-APItest/t/exception.t XS::APItest extension +ext/XS-APItest/t/extend.t test EXTEND() macro ext/XS-APItest/t/fetch_pad_names.t Tests for UTF8 names in pad ext/XS-APItest/t/gotosub.t XS::APItest: tests goto &xsub and hints ext/XS-APItest/t/grok.t XS::APItest: tests for grok* functions |