Repository navigation
Undefined behaviour in get_large_size_class() #133
Description
As mentioned there, I'm opening this issue to discuss another finding I investigated following static code analysis of hardened_malloc.
If we can reach the following definition in get_large_size_class() with 0 < size < 5, we trigger either an undefined behaviour (call to __builtin_clzl(0)) or an underflow of spacing_shift:
size_t spacing_shift = 64 - __builtin_clzl(size - 1) - 3;Then, in the underflow case and on the next line, spacing_shift would be greater than the width of the left operand, which is an undefined behaviour in C:
size_t spacing_class = 1ULL << spacing_shift;I've been able to hit this via two different paths, with SLAB_CANARY set to false:
alloc_aligned(), for instance from a call toh_aligned_alloc(8192, 2);h_free_sized(p, expected_size)called with0 < expected_size < 5andppointing outside the slab region.
Activity
thestinger commentedon Feb 16, 2021
That needs to be fixed. It doesn't appear that it could actually cause a problem in practice but the code should be clean of undefined behavior for well-defined usage of the API.
This is avoidable, but the caller has already done something that is going to end up being considered undefined when this API is standardized in the C standard, which is likely going to be happening. So we can avoid doing anything more undefined, but simply the fact that the caller has done this will be considered undefined in the future.
thestinger commentedon Feb 16, 2021
i.e. passing an invalid pointer and/or expected size to
free_sizedwill be considered undefined and most malloc implementations will use free_sized for optimization so they'll be trusting that they were passed the correct size rather than using it to add sanity checking.thestinger commentedon Feb 16, 2021
This should be resolved by 29b0964.
tsautereau-anssi commentedon Feb 16, 2021
You're welcome.
thestinger commentedon Feb 16, 2021
Interestingly, GCC and Clang handle the undefined behavior for the 1st case differently. Since this can cause an incorrect out-of-memory error to be reported for Clang in that kind of edge case (unlikely to be used in the real world, but still valid), it's probably worth tagging a new release for it.