Skip to content

Simplify and fix SAFE_HEAP #24291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
RReverser merged 9 commits into from
May 13, 2025
Merged

Simplify and fix SAFE_HEAP #24291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a8874fa
Add failing test for i64 + SAFE_HEAP
RReverser May 9, 2025
946bfd1
Simplify `SAFE_HEAP` implementation
RReverser May 9, 2025
ba4146b
Rebaseline
RReverser May 9, 2025
34b458e
Fixups
RReverser May 9, 2025
d3f196e
Format
RReverser May 9, 2025
eba471f
Also remove obsolete (get|set)Value_safe helpers
RReverser May 9, 2025
2a0b10c
Reset codesize files
RReverser May 9, 2025
00de2cf
Skip test on SAFE_HEAP + ASan together
RReverser May 9, 2025
7946613
Fix test
RReverser May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions src/lib/libgetvalue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,6 @@ var LibraryMemOps = {
* @param {string} type
*/`,
$getValue: getValueImpl,

#if SAFE_HEAP
// The same as the above two functions, but known to the safeHeap pass
// in tools/acorn-optimizer.mjs. The heap accesses within these two
// functions will *not* get re-written.
// Note that we do not use the alias mechanism here since we need separate
// instances of above setValueImpl/getValueImpl functions.
$setValue_safe__internal: true,
$setValue_safe: setValueImpl,

$getValue_safe__internal: true,
$getValue_safe: getValueImpl,
#endif
};

addToLibrary(LibraryMemOps);
76 changes: 12 additions & 64 deletions src/runtime_safe_heap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,91 +8,39 @@
#error "should only be inclded in SAFE_HEAP mode"
#endif

/** @param {number|boolean=} isFloat */
function getSafeHeapType(bytes, isFloat) {
switch (bytes) {
case 1: return 'i8';
case 2: return 'i16';
case 4: return isFloat ? 'float' : 'i32';
case 8: return isFloat ? 'double' : 'i64';
default: abort(`getSafeHeapType() invalid bytes=${bytes}`);
}
}

#if SAFE_HEAP_LOG
var SAFE_HEAP_COUNTER = 0;
#endif

/** @param {number|boolean=} isFloat */
function SAFE_HEAP_STORE(dest, value, bytes, isFloat) {
function SAFE_HEAP_INDEX(arr, idx, action) {
#if CAN_ADDRESS_2GB
dest >>>= 0;
idx >>>= 0;
#endif
const bytes = arr.BYTES_PER_ELEMENT;
const dest = idx * bytes;
#if SAFE_HEAP_LOG
dbg('SAFE_HEAP store: ' + [dest, value, bytes, isFloat, SAFE_HEAP_COUNTER++]);
#endif
if (dest <= 0) abort(`segmentation fault storing ${bytes} bytes to address ${dest}`);
#if SAFE_HEAP == 1
if (dest % bytes !== 0) abort(`alignment error storing to address ${dest}, which was expected to be aligned to a multiple of ${bytes}`);
#else
if (dest % bytes !== 0) warnOnce(`alignment error in a memory store operation, alignment was a multiple of ${(((dest ^ (dest-1)) >> 1) + 1)}, but was was expected to be aligned to a multiple of ${bytes}`);
Copy link
Member

@kripken kripken May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to the alignment checks?

Copy link
Collaborator Author

@RReverser RReverser May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer necessary because we pass in an index in the corresponding array instead of pre-multiplied pointer.

Those were already guaranteed not to be hit since we always did transform like HEAP32[i] -> SAFE_HEAP_LOAD(i * 4, 4, ...) and then function would just check that i * 4 is divisible by 4, which is always true, but now it's just more obvious since we skip the premultiplication.

Copy link
Member

@kripken kripken May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I was also concerned about directly-emitted SAFE_HEAP_* calls from parseTools (not from the acorn transform), but I see we removed those a while ago, which I forgot...

Copy link
Collaborator Author

@RReverser RReverser May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I did also notice that

emscripten/site/source/docs/api_reference/preamble.js.rst

Lines 381 to 391 in 976b43d

function SAFE_HEAP_CLEAR(dest)
function SAFE_HEAP_ACCESS(dest, type, store, ignore, storeValue)
function SAFE_HEAP_STORE(dest, value, type, ignore)
function SAFE_HEAP_LOAD(dest, type, unsigned, ignore)
function SAFE_HEAP_COPY_HISTORY(dest, src)
function SAFE_HEAP_FILL_HISTORY(from, to, type)
function getSafeHeapType(bytes, isFloat)
function SAFE_HEAP_STORE(dest, value, bytes, isFloat)
function SAFE_HEAP_LOAD(dest, bytes, isFloat, unsigned)
function SAFE_FT_MASK(value, mask)
function CHECK_OVERFLOW(value, bits, ignore, sig)
lists quite a few methods that don't (no longer?) exist, and probably misses bunch of new ones instead.

Maybe that section should be just removed at this point instead of trying in sync, since it's pretty outdated and not rendered in docs anyway?

Copy link
Member

@kripken kripken May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, yeah, best to just remove that bit, good find.

Copy link
Collaborator Author

@RReverser RReverser May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, I think we saw that that JS-side bug is long-standing and never caused any issues, so it doesn't feel urgent to fix.

Fair enough. I was mainly curious in context of those no-op checks we are commenting on, as I assume they used to do something useful in the long past, so technically a regression happened at some point... but idk when. As you say, it's probably not urgent.

If the input code contains HEAP32[x] then by construction x must be 4-byte aligned, no?

Problem I'm thinking about, a lot of code has form like HEAP32[somePtr >> 2] and such, whether handcrafted or generated via makeGetValue, and in those cases there is no guarantee somePtr is already correctly aligned.

JS transform could detect at least this common form of access and check the alignment. Or maybe at least makeGetValue/getHeapOffset itself could insert those checks when SAFE_HEAP is enabled.

Copy link
Collaborator

@sbc100 sbc100 May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but what about this?

// a JS function that is called from wasm, and given a (maybe unaligned) pointer
function jsHelper(ptr) {
  return HEAP32[ptr >> 2];
}

But I don't think the old code handled this did it? It just checked the value of ptr >> 2 at runtime, right? So there is no regression here is there?

Copy link
Collaborator

@sbc100 sbc100 May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS transform could detect at least this common form of access and check the alignment. Or maybe at least makeGetValue/getHeapOffset itself could insert those checks when SAFE_HEAP is enabled.

To be clear neither the old new the version the JS transform do this though do they?

Copy link
Collaborator Author

@RReverser RReverser May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear neither the old new the version the JS transform do this though do they?

If you mean before/after this PR, then yeah.

But judging by the removed code, it used to in the long past. And, well, even if it didn't, just saying it would be a useful addition.

Copy link
Member

@kripken kripken May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this isn't a recent regression. But it may have regressed a long time ago, or never worked... Anyhow, alignment checks from JS are pretty minor, not worth worrying about.

dbg(`SAFE_HEAP ${action}: ${[arr.constructor.name, idx, SAFE_HEAP_COUNTER++]}`);
#endif
if (idx <= 0) abort(`segmentation fault ${action} ${bytes} bytes at address ${dest}`);
#if EXIT_RUNTIME
if (runtimeInitialized && !runtimeExited) {
#else
if (runtimeInitialized) {
#endif
var brk = _sbrk(0);
if (dest + bytes > brk) abort(`segmentation fault, exceeded the top of the available dynamic heap when storing ${bytes} bytes to address ${dest}. DYNAMICTOP=${brk}`);
if (dest + bytes > brk) abort(`segmentation fault, exceeded the top of the available dynamic heap when ${action} ${bytes} bytes at address ${dest}. DYNAMICTOP=${brk}`);
if (brk < _emscripten_stack_get_base()) abort(`brk >= _emscripten_stack_get_base() (brk=${brk}, _emscripten_stack_get_base()=${_emscripten_stack_get_base()})`); // sbrk-managed memory must be above the stack
if (brk > wasmMemory.buffer.byteLength) abort(`brk <= wasmMemory.buffer.byteLength (brk=${brk}, wasmMemory.buffer.byteLength=${wasmMemory.buffer.byteLength})`);
}
setValue_safe(dest, value, getSafeHeapType(bytes, isFloat));
return value;
}
function SAFE_HEAP_STORE_D(dest, value, bytes) {
return SAFE_HEAP_STORE(dest, value, bytes, true);
return idx;
}

/** @param {number|boolean=} isFloat */
function SAFE_HEAP_LOAD(dest, bytes, unsigned, isFloat) {
#if CAN_ADDRESS_2GB
dest >>>= 0;
#endif
if (dest <= 0) abort(`segmentation fault loading ${bytes} bytes from address ${dest}`);
#if SAFE_HEAP == 1
if (dest % bytes !== 0) abort(`alignment error loading from address ${dest}, which was expected to be aligned to a multiple of ${bytes}`);
#else
if (dest % bytes !== 0) warnOnce(`alignment error in a memory load operation, alignment was a multiple of ${(((dest ^ (dest-1)) >> 1) + 1)}, but was was expected to be aligned to a multiple of ${bytes}`);
#endif
#if EXIT_RUNTIME
if (runtimeInitialized && !runtimeExited) {
#else
if (runtimeInitialized) {
#endif
var brk = _sbrk(0);
if (dest + bytes > brk) abort(`segmentation fault, exceeded the top of the available dynamic heap when loading ${bytes} bytes from address ${dest}. DYNAMICTOP=${brk}`);
if (brk < _emscripten_stack_get_base()) abort(`brk >= _emscripten_stack_get_base() (brk=${brk}, _emscripten_stack_get_base()=${_emscripten_stack_get_base()})`); // sbrk-managed memory must be above the stack
if (brk > wasmMemory.buffer.byteLength) abort(`brk <= wasmMemory.buffer.byteLength (brk=${brk}, wasmMemory.buffer.byteLength=${wasmMemory.buffer.byteLength})`);
}
var type = getSafeHeapType(bytes, isFloat);
var ret = getValue_safe(dest, type);
if (unsigned) ret = unSign(ret, parseInt(type.slice(1), 10));
#if SAFE_HEAP_LOG
dbg('SAFE_HEAP load: ' + [dest, ret, bytes, isFloat, unsigned, SAFE_HEAP_COUNTER++]);
#endif
return ret;
}
function SAFE_HEAP_LOAD_D(dest, bytes, unsigned) {
return SAFE_HEAP_LOAD(dest, bytes, unsigned, true);
function SAFE_HEAP_LOAD(arr, idx) {
return arr[SAFE_HEAP_INDEX(arr, idx, 'loading')];
}

function SAFE_FT_MASK(value, mask) {
var ret = value & mask;
if (ret !== value) {
abort(`Function table mask error: function pointer is ${value} which is masked by ${mask}, the likely cause of this is that the function pointer is being called by the wrong type.`);
}
return ret;
function SAFE_HEAP_STORE(arr, idx, value) {
return arr[SAFE_HEAP_INDEX(arr, idx, 'storing')] = value;
}

function segfault() {
Expand Down
58 changes: 25 additions & 33 deletions test/js_optimizer/safeHeap-output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,65 +1,57 @@
SAFE_HEAP_STORE(x, 1, 1);
SAFE_HEAP_STORE(HEAP8, x, 1);

SAFE_HEAP_STORE(x * 2, 2, 2);
SAFE_HEAP_STORE(HEAP16, x, 2);

SAFE_HEAP_STORE(x * 4, 3, 4);
SAFE_HEAP_STORE(HEAP32, x, 3);

SAFE_HEAP_STORE(x, 4, 1);
SAFE_HEAP_STORE(HEAPU8, x, 4);

SAFE_HEAP_STORE(x * 2, 5, 2);
SAFE_HEAP_STORE(HEAPU16, x, 5);

SAFE_HEAP_STORE(x * 4, 6, 4);
SAFE_HEAP_STORE(HEAPU32, x, 6);

SAFE_HEAP_STORE_D(x * 4, 7, 4);
SAFE_HEAP_STORE(HEAPF32, x, 7);

SAFE_HEAP_STORE_D(x * 8, 8, 8);
SAFE_HEAP_STORE(HEAPF64, x, 8);

SAFE_HEAP_STORE(x * 8, 9n, 8);
SAFE_HEAP_STORE(HEAP64, x, 9n);

SAFE_HEAP_STORE(x * 8, 10n, 8);
SAFE_HEAP_STORE(HEAPU64, x, 10n);

a1 = SAFE_HEAP_LOAD(x, 1, 0);
a1 = SAFE_HEAP_LOAD(HEAP8, x);

a2 = SAFE_HEAP_LOAD(x * 2, 2, 0);
a2 = SAFE_HEAP_LOAD(HEAP16, x);

a3 = SAFE_HEAP_LOAD(x * 4, 4, 0);
a3 = SAFE_HEAP_LOAD(HEAP32, x);

a4 = SAFE_HEAP_LOAD(x, 1, 1);
a4 = SAFE_HEAP_LOAD(HEAPU8, x);

a5 = SAFE_HEAP_LOAD(x * 2, 2, 1);
a5 = SAFE_HEAP_LOAD(HEAPU16, x);

a6 = SAFE_HEAP_LOAD(x * 4, 4, 1);
a6 = SAFE_HEAP_LOAD(HEAPU32, x);

a7 = SAFE_HEAP_LOAD_D(x * 4, 4, 0);
a7 = SAFE_HEAP_LOAD(HEAPF32, x);

a8 = SAFE_HEAP_LOAD_D(x * 8, 8, 0);
a8 = SAFE_HEAP_LOAD(HEAPF64, x);

a9 = SAFE_HEAP_LOAD(x * 8, 8, 0);
a9 = SAFE_HEAP_LOAD(HEAP64, x);

a10 = SAFE_HEAP_LOAD(x * 8, 8, 1);
a10 = SAFE_HEAP_LOAD(HEAPU64, x);

foo = SAFE_HEAP_STORE(1337, 42, 1);
foo = SAFE_HEAP_STORE(HEAPU8, 1337, 42);

SAFE_HEAP_LOAD(bar(SAFE_HEAP_LOAD_D(5 * 8, 8, 0)) * 2, 2, 0);
SAFE_HEAP_LOAD(HEAP16, bar(SAFE_HEAP_LOAD(HEAPF64, 5)));

SAFE_HEAP_STORE_D(x * 4, SAFE_HEAP_LOAD(y * 4, 4, 0), 4);
SAFE_HEAP_STORE(HEAPF32, x, SAFE_HEAP_LOAD(HEAP32, y));

function SAFE_HEAP_FOO(ptr) {
return HEAP8[ptr];
}

function setValue_safe(ptr) {
return HEAP8[ptr];
}

function getValue_safe(ptr) {
return HEAP8[ptr];
}

function somethingElse() {
return SAFE_HEAP_LOAD(ptr, 1, 0);
return SAFE_HEAP_LOAD(HEAP8, ptr);
}

HEAP8.length;

SAFE_HEAP_LOAD(length, 1, 0);
SAFE_HEAP_LOAD(HEAP8, length);
6 changes: 0 additions & 6 deletions test/js_optimizer/safeHeap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,6 @@ HEAPF32[x] = HEAP32[y];
function SAFE_HEAP_FOO(ptr) {
return HEAP8[ptr];
}
function setValue_safe(ptr) {
return HEAP8[ptr];
}
function getValue_safe(ptr) {
return HEAP8[ptr];
}

// but do handle everything else
function somethingElse() {
Expand Down
11 changes: 9 additions & 2 deletions test/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7493,8 +7493,15 @@ def test_embind_dynamic_initialization(self):
self.do_run_in_out_file_test('embind/test_dynamic_initialization.cpp')

@no_wasm2js('wasm_bigint')
def test_embind_i64_val(self):
@parameterized({
'': (False,),
'safe_heap': (True,),
})
def test_embind_i64_val(self, safe_heap):
self.set_setting('WASM_BIGINT')
if safe_heap and '-fsanitize=address' in self.emcc_args:
self.skipTest('asan does not work with SAFE_HEAP')
self.set_setting('SAFE_HEAP', safe_heap)
self.emcc_args += ['-lembind']
self.node_args += shared.node_bigint_flags(self.get_nodejs())
self.do_run_in_out_file_test('embind/test_i64_val.cpp', assert_identical=True)
Expand Down Expand Up @@ -8965,7 +8972,7 @@ def test_asan_modularized_with_closure(self):
def test_safe_heap_user_js(self):
self.set_setting('SAFE_HEAP')
self.do_runf('core/test_safe_heap_user_js.c',
expected_output=['Aborted(segmentation fault storing 1 bytes to address 0)'], assert_returncode=NON_ZERO)
expected_output=['Aborted(segmentation fault storing 1 bytes at address 0)'], assert_returncode=NON_ZERO)

def test_safe_stack(self):
self.set_setting('STACK_OVERFLOW_CHECK', 2)
Expand Down
2 changes: 1 addition & 1 deletion test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12048,7 +12048,7 @@ def test_safe_heap_2(self):
def test_safe_heap_log(self):
self.set_setting('SAFE_HEAP')
self.set_setting('SAFE_HEAP_LOG')
self.do_runf('hello_world.c', 'SAFE_HEAP load: ')
self.do_runf('hello_world.c', 'SAFE_HEAP loading: ')

def test_mini_printfs(self):
def test(code):
Expand Down
Loading