-
Notifications
You must be signed in to change notification settings - Fork 542
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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));
- Loading branch information
Showing
8 changed files
with
152 additions
and
11 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ use strict; | |
use warnings; | ||
use Carp; | ||
|
||
our $VERSION = '0.75'; | ||
our $VERSION = '0.76'; | ||
|
||
require XSLoader; | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
#!perl | ||
# | ||
# Test stack expansion macros: EXTEND() etc, especially for edge cases | ||
# where the count wraps to a native value or gets truncated. | ||
# | ||
# Some of these tests aren't really testing; they are however exercising | ||
# edge cases, which other tools like ASAN may then detect problems with. | ||
# In particular, test_EXTEND() does *(p+n) = NULL and *PL_stack_max = NULL | ||
# before returning, to help such tools spot errors. | ||
# | ||
# Also, it doesn't test large but legal grow requests; only ridiculously | ||
# large requests that are guaranteed to wrap. | ||
|
||
use Test::More; | ||
use Config; | ||
use XS::APItest qw(test_EXTEND); | ||
|
||
plan tests => 48; | ||
|
||
my $uvsize = $Config::Config{uvsize}; # sizeof(UV) | ||
my $sizesize = $Config::Config{sizesize}; # sizeof(Size_t) | ||
|
||
# The first arg to test_EXTEND() is the SP to use in EXTEND(), treated | ||
# as an offset from PL_stack_max. So extend(-1, 1, $use_ss) shouldn't | ||
# call Perl_stack_grow(), while extend(-1, 2, $use_ss) should. | ||
# Exercise offsets near to PL_stack_max to detect edge cases. | ||
# Note that having the SP pointer beyond PL_stack_max is legal. | ||
|
||
for my $offset (-1, 0, 1) { | ||
|
||
# treat N as either an IV or a SSize_t | ||
for my $use_ss (0, 1) { | ||
|
||
# test with N in range -1 .. 3; only the -1 should panic | ||
|
||
eval { test_EXTEND($offset, -1, $use_ss) }; | ||
like $@, qr/panic: .*negative count/, "test_EXTEND($offset, -1, $use_ss)"; | ||
|
||
for my $n (0,1,2,3) { | ||
eval { test_EXTEND($offset, $n, $use_ss) }; | ||
is $@, "", "test_EXTEND($offset, $n, $use_ss)"; | ||
} | ||
|
||
# some things can wrap if the int size is greater than the ptr size | ||
|
||
SKIP: { | ||
skip "Not small ptrs", 3 if $use_ss || $uvsize <= $sizesize; | ||
|
||
# 0xffff... wraps to -1 | ||
eval { test_EXTEND($offset, (1 << 8*$sizesize)-1, $use_ss) }; | ||
like $@, qr/panic: .*negative count/, | ||
"test_EXTEND(-1, SIZE_MAX, $use_ss)"; | ||
|
||
# 0x10000... truncates to zero; | ||
# but the wrap-detection code converts it to -1 to force a panic | ||
eval { test_EXTEND($offset, 1 << 8*$sizesize, $use_ss) }; | ||
like $@, qr/panic: .*negative count/, | ||
"test_EXTEND(-1, SIZE_MAX+1, $use_ss)"; | ||
|
||
# 0x1ffff... truncates and then wraps to -1 | ||
eval { test_EXTEND($offset, (1 << (8*$sizesize+1))-1, $use_ss) }; | ||
like $@, qr/panic: .*negative count/, | ||
"test_EXTEND(-1, 2*SIZE_MAX-1, $use_ss)"; | ||
|
||
|
||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters