emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.4k stars 3.26k forks source link

SAFE_HEAP should default maximum memory to 1 block less than 2GB #21557

Closed jezell closed 1 week ago

jezell commented 4 months ago

When allow memory growth and SAFE_HEAP are enabled, SAFE_HEAP will integer overflow when checking pointers at the very end of the heap causing all checks at the end of the heap to improperly fail due to seeing a negative number in the comparison.

sbc100 commented 4 months ago

IIUC SAFE_HEAP always check based on your MAXIMUM_MEMORY setting? Are you running into this issue in your code? If so can you share what setting you using for INITIAL_MEMORY and MAXIMUM_MEMORY.

Presumably another solution to this would be use treat all address as unsigned (e.g. by doing >>> 0 prior to the check).

sbc100 commented 4 months ago

It looks like we already treat address as unsigned in CAN_ADDRESS_2GB mode:

https://github.com/emscripten-core/emscripten/blob/0c47091cc9490c5f8476f7ea590eaedfe00945f0/src/runtime_safe_heap.js#L28-L30

Perhaps we should just make this unconditional?

I guess that means you are not in CAN_ADDRESS_2GB mode (i.e. you didn't specify a MAX or over 2gb)?

sbc100 commented 4 months ago

Can you try this fix: https://github.com/emscripten-core/emscripten/pull/21560

kripken commented 4 months ago

SAFE_HEAP will integer overflow when checking pointers at the very end of the heap

The JS version of this looks ok, as it's on JS Numbers, so it's in 53 bits of precision:

https://github.com/emscripten-core/emscripten/blob/cb0d16c6de9778cb95016cc5e06a77b0aa1d83cb/src/runtime_safe_heap.js#L46

The wasm version emits stuff like this:

 (func $SAFE_HEAP_LOAD_i32_1_1 (param $0 i32) (param $1 i32) (result i32)
  (local $2 i32)
  (local.set $2
   (i32.add
    (local.get $0)
    (local.get $1)
   )
  )
  (if
   (i32.or
    (i32.eq
     (local.get $2)
     (i32.const 0)
    )
    (i32.gt_u
     (i32.add
      (local.get $2)
      (i32.const 1)
     )
     (i32.load
      (call $emscripten_get_sbrk_ptr)
     )
    )
   )
   (then
    (call $segfault)
   )
  )
  (i32.load8_s
   (local.get $2)
  )
 )

So that first add can indeed overflow, good catch. We need an extra check before that add, in Binaryen's SafeHeap pass.