Undefined behaviour in get_large_size_class()

[tsautereau-anssi]

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 to h_aligned_alloc(8192, 2);
• h_free_sized(p, expected_size) called with 0 < expected_size < 5 and p pointing outside the slab region.

[thestinger]

alloc_aligned(), for instance from a call to h_aligned_alloc(8192, 2);

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.

h_free_sized(p, expected_size) called with 0 < expected_size < 5 and p pointing outside the slab region.

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]

i.e. passing an invalid pointer and/or expected size to free_sized will 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]

This should be resolved by 29b0964.

[tsautereau-anssi]

You're welcome.

[thestinger]

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.