laverdet / isolated-vm

Secure & isolated JS environments for nodejs
ISC License
2.2k stars 154 forks source link

Underflow + false positive OOM in memory usage tracking #374

Closed mwpenny closed 1 year ago

mwpenny commented 1 year ago

Reproduction steps

Run the following sample code:

const ivm = require('isolated-vm');

const isolate = new ivm.Isolate();
const context = isolate.createContextSync();
context.global.setSync('ivm', ivm);

const func = isolate.compileScriptSync(
    'new ivm.Reference(function func() { ' +
    '    let ret = 0;' +
    '    for (let i = 0; i < 100; ++i) {' +
    '        let float = new Float64Array(1);' +
    '        let bytes = new Uint8Array(float.buffer);' +
    '        ret += bytes[0];' +
    '    }' +
    '    return ret;' +
    '})'
).runSync(context);

for (let i = 0; i < 1000000; ++i) {
    func.applySync();
}

Note that I couldn't reproduce it with new Uint8Array(float). I had to use new Uint8Array(float.buffer).

Expected behavior

Code terminates gracefully after some time.

Actual behavior

An out of memory error is thrown:

Error: Isolate was disposed during execution due to memory limit at Reference.applySync () at Object. (C:\path\to\script.js) at Module._compile (node:internal/modules/cjs/loader:1198:14) at Object.Module._extensions..js (node:internal/modules/cjs/loader:1252:10) at Module.load (node:internal/modules/cjs/loader:1076:32) at Function.Module._load (node:internal/modules/cjs/loader:911:12) at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12) at node:internal/main/run_main_module:22:47 at (\<isolated-vm boundary>) at C:\path\to\script.js

The amount of time it takes to trigger the error varies. I can usually get it to occur within a few seconds. Sometimes it takes longer (~1-2 minutes), and occasionally it doesn't happen at all and the code runs as expected. In all cases, memory usage stays stable.

I have reproduced the issue on both Windows and Linux using the latest version (4.5.0).

Additional information

I investigated this a bit myself and added some logging in MarkSweepCompactEpilogue() when hit_memory_limit is about to be set to true:

heap.used_heap_size = 653192 that->extra_allocated_memory = 18446744073708260368 that->memory_limit = 134217728 that->misc_memory_size = 3145728 total_memory = 18446744073708913560 memory_limit = 137363456

that->extra_allocated_memory is abnormally high (0xFFFFFFFFFFEC4C10 bytes == ~17179869183 GB). It is also -1291248 when interpreted as a signed number. This causes a false positive in the total_memory > memory_limit check.

I added more logging and it looks to be caused by freeing of untracked memory in LimitedAllocator::Free(), causing an underflow of extra_allocated_memory. I dumped the stack and it was:

0: ivm::LimitedAllocator::Free 1: v8::internal::BackingStore::~BackingStore 2: v8::internal::wasm::JumpTableAssembler::SlotOffsetToIndex 3: v8::internal::ArrayBufferList::Contains 4: v8::internal::BaseSpace::GetSpaceName 5: node::TriggerNodeReport 6: uv_poll_stop 7: v8::internal::compiler::RepresentationChanger::Uint32OverflowOperatorFor 8: BaseThreadInitThunk 9: RtlUserThreadStart

Sorry, I don't have line numbers as I had trouble running this when compiled in debug.

The fact that BackingStore::~BackingStore is involved makes sense to me since the reproduction case requires ArrayBuffers/TypedArrays.

I hope this helps, and let me know if you need more information!

mwpenny commented 1 year ago

Do you have a rough idea on when this will make it to NPM? We're hitting this quite regularly.

Thanks!