SafeHeap: Handle overflows when adding the pointer and the size

[kripken]

E.g. loading 4 bytes from 2^32 - 2 should error: 2 bytes are past the maximum
address. Before this PR we added 2^32 - 2 + 4 and overflowed to 2, which we
saw as a low and safe address. This PR adds an extra check for an overflow in
that add.

Fixes emscripten-core/emscripten#21557

[kripken]

(Testing this here is not easy, as we don't have a full emscripten output with sbrk_ptr support etc. But we can add an Emscripten test.)

[sbc100]

Looks like a lot of extra generated code. Do you think its worth it?

Also, it not obvious to me how those test got updated. Presumably you didn't update them by hand. Can you mention the command you used to update them?

[kripken]

I think this is worth fixing. Safe-heap builds can afford to be a little larger and slower, even if this particular corner case is very rare.

The test update here is 100% automatic. See the last section in the readme testing notes, specifically I ran ./scripts/auto_update_tests.py wasm-opt.

[sbc100]

I think this is worth fixing. Safe-heap builds can afford to be a little larger and slower, even if this particular corner case is very rare.

Sure but isn't this change almost doubling the overhead, for the sake of a fairly rare case? Maybe its not rare? Or maybe i'm not understanding the proportional impact of this chage?

[kripken]

The overhead is significant I'm guessing, but not double. A typical safe-heap method here is 18 instructions, and this adds 5. But much of the overhead is likely in the call to the safe-heap method anyhow. Still, maybe it's worth measuring this, as e.g. if this prevents inlining of these methods (assuming they were inlined before) then maybe it is significant actually. I can do some measurements.

[sbc100]

The overhead is significant I'm guessing, but not double. A typical safe-heap method here is 18 instructions, and this adds 5. But much of the overhead is likely in the call to the safe-heap method anyhow. Still, maybe it's worth measuring this, as e.g. if this prevents inlining of these methods (assuming they were inlined before) then maybe it is significant actually. I can do some measurements.

No worries, that sounds reasonable to me. If folks complain that its prohibitive we can always reasses.

[kripken]

Measuring this, these functions were already too big to get inlined, so that didn't change. On fannkuch I see a negligible code size change (much less than 1%), though speed does decrease by 5%. So this is a regression, but in this type of build I think it's ok when it improves the correctness of what we are checking, just like more checks make UBSan builds slower.

If that 5% mattered then we could optimize this actually. Atm we emit

if (overflow) segfault();
if (out of bounds) segfault();

And we really just need one if here. Fixing that would require some refactoring here.

I did see that changing to

if (overflow) { segfault(); unreachable } // add an unreachable here
if (out of bounds) segfault();

removes the regression here on speed. Adding that unreachable byte seems to help the VM, perhaps by telling it no control flow merges happen later. Anyhow, it seems we could easily speed this up with some work, but not sure if it's worthwhile atm.

[sbc100]

lgtm either way

[kripken]

This needed some refactoring anyhow - I only put this in the loads, but the stores need it too. In the stores this added a bit more overhead to 6.5%.

I added those unreachables and the overhead is down to 1.5%, which is likely not noticeable, and seems useful anyhow. These are basically noreturn calls, which clang would append an unreachable to as well.

[sbc100]

Should we land this?

[kripken]

I'm not sure. 6.5% overhead is maybe not a big deal in a slow build anyhow, but it is for quite small gain. What do you think?

[sbc100]

I don't have a strong opinion.. just trying to close some issues :)

[kripken]

Ok, let's land it. The point of SAFE_HEAP is to catch memory bugs, and this situation is a memory bug.