Fix stale getArrayBuffer view and host leaks on failure paths (follow-up to #248)

packages/quickjs-emscripten-core/src/context.ts

@@ -127,9 +127,11 @@ class ContextMemory extends ModuleMemory implements Disposable {
   }
 
   consumeJSCharPointer(ptr: JSBorrowedCharPointer): string {
-    const str = this.module.UTF8ToString(ptr)
-    this.ffi.QTS_FreeCString(this.ctx.value, ptr)
-    return str
+    try {
+      return this.module.UTF8ToString(ptr)
+    } finally {
+      this.ffi.QTS_FreeCString(this.ctx.value, ptr)
+    }
   }
 
   heapValueHandle(ptr: JSValuePointer, extraDispose?: () => void): JSValue {
@@ -448,6 +450,13 @@ export class QuickJSContext
     const array = new Uint8Array(buffer)
     const handle = this.memory.newHeapBufferPointer(array)
     const ptr = this.ffi.QTS_NewArrayBuffer(this.ctx.value, handle.value.pointer, array.length)
+    const errorPtr = this.ffi.QTS_ResolveException(this.ctx.value, ptr)
+    if (errorPtr) {
+      // QuickJS only takes ownership of the host buffer when it succeeds in creating the ArrayBuffer.
+      handle.dispose()
+      this.ffi.QTS_FreeValuePointer(this.ctx.value, ptr)
+      return this.unwrapResult(this.fail(this.memory.heapValueHandle(errorPtr)))
+    }
     return this.memory.heapValueHandle(ptr)
   }
 
@@ -487,9 +496,9 @@ export class QuickJSContext
         mutablePointerArray.value.ptr,
       )
       const promiseHandle = this.memory.heapValueHandle(promisePtr)
-      const [resolveHandle, rejectHandle] = Array.from(mutablePointerArray.value.typedArray).map(
-        (jsvaluePtr) => this.memory.heapValueHandle(jsvaluePtr as any),
-      )
+      const [resolveHandle, rejectHandle] = Array.from(
+        mutablePointerArray.value.typedArray.value,
+      ).map((jsvaluePtr) => this.memory.heapValueHandle(jsvaluePtr as any))
       return new QuickJSDeferredPromise({
         context: this,
         promiseHandle,
@@ -806,7 +815,9 @@ export class QuickJSContext
   }
 
   /**
-   * Coverts `handle` to a JavaScript ArrayBuffer
+   * Coverts `handle` to a JavaScript ArrayBuffer.
+   *
+   * The returned bytes are a copy that stays valid however much the WASM heap grows afterwards.
    */
   getArrayBuffer(handle: QuickJSHandle): Lifetime<Uint8Array> {
     this.runtime.assertOwned(handle)
@@ -815,9 +826,12 @@ export class QuickJSContext
     if (!ptr) {
       throw new Error("Couldn't allocate memory to get ArrayBuffer")
     }
-    return new Lifetime(this.module.HEAPU8.subarray(ptr, ptr + len), undefined, () =>
-      this.module._free(ptr),
-    )
+    try {
+      // A view into the heap would be detached by the next memory growth, so copy out.
+      return new Lifetime<Uint8Array>(this.module.HEAPU8.slice(ptr, ptr + len))
+    } finally {
+      this.module._free(ptr)
+    }
   }
 
   /**
@@ -994,7 +1008,7 @@ export class QuickJSContext
     if (status < 0) {
       return undefined
     }
-    return this.uint32Out.value.typedArray[0]
+    return this.uint32Out.value.typedArray.value[0]
   }
 
   /**
@@ -1052,9 +1066,9 @@ export class QuickJSContext
       if (errorPtr) {
         return this.fail(this.memory.heapValueHandle(errorPtr))
       }
-      const len = this.uint32Out.value.typedArray[0]
-      const ptr = outPtr.value.typedArray[0]
-      const pointerArray = new Uint32Array(this.module.HEAP8.buffer, ptr, len)
+      const len = this.uint32Out.value.typedArray.value[0]
+      const ptr = outPtr.value.typedArray.value[0]
+      const pointerArray = new Uint32Array(this.module.HEAPU8.buffer, ptr, len)
       const handles = Array.from(pointerArray).map((ptr) =>
         this.memory.heapValueHandle(ptr as JSValuePointer),
       )

packages/quickjs-emscripten-core/src/memory.ts

@@ -29,9 +29,39 @@ export interface TypedArrayConstructor<T> {
   BYTES_PER_ELEMENT: number
 }
 
+/**
+ * A TypedArray view into WASM memory that automatically refreshes when memory grows.
+ *
+ * When Emscripten's WASM memory grows (due to -sALLOW_MEMORY_GROWTH), all existing
+ * TypedArray views become detached because the underlying ArrayBuffer is replaced.
+ * This class lazily recreates the view when the buffer changes.
+ *
+ * @private
+ */
+export class RefreshableTypedArray<T extends TypedArray> {
+  private cachedArray: T | undefined
+  private lastBuffer: ArrayBufferLike | undefined
+
+  constructor(
+    private readonly module: EitherModule,
+    private readonly kind: TypedArrayConstructor<T>,
+    private readonly ptr: number,
+    private readonly length: number,
+  ) {}
+
+  get value(): T {
+    const currentBuffer = this.module.HEAPU8.buffer
+    if (this.cachedArray === undefined || this.lastBuffer !== currentBuffer) {
+      this.cachedArray = new this.kind(currentBuffer, this.ptr, this.length)
+      this.lastBuffer = currentBuffer
+    }
+    return this.cachedArray
+  }
+}
+
 /** @private */
 export type HeapTypedArray<JS extends TypedArray, C extends number> = Lifetime<{
-  typedArray: JS
+  typedArray: RefreshableTypedArray<JS>
   ptr: C
 }>
 
@@ -54,18 +84,17 @@ export class ModuleMemory {
     kind: TypedArrayConstructor<JS>,
     length: number,
   ): HeapTypedArray<JS, C> {
-    const zeros = new kind(new Array(length).fill(0))
-    const numBytes = zeros.length * zeros.BYTES_PER_ELEMENT
+    const numBytes = length * kind.BYTES_PER_ELEMENT
     const ptr = this.module._malloc(numBytes) as C
-    const typedArray = new kind(this.module.HEAPU8.buffer, ptr, length)
-    typedArray.set(zeros)
+    const typedArray = new RefreshableTypedArray(this.module, kind, ptr, length)
+    typedArray.value.fill(0)
     return new Lifetime({ typedArray, ptr }, undefined, (value) => this.module._free(value.ptr))
   }
 
   // TODO: shouldn't this be Uint32 instead of Int32?
   newMutablePointerArray<T extends number>(
     length: number,
-  ): Lifetime<{ typedArray: Int32Array<ArrayBuffer>; ptr: T }> {
+  ): Lifetime<{ typedArray: RefreshableTypedArray<Int32Array<ArrayBuffer>>; ptr: T }> {
     return this.newTypedArray(Int32Array, length)
   }
 
@@ -87,8 +116,10 @@ export class ModuleMemory {
   }
 
   consumeHeapCharPointer(ptr: OwnedHeapCharPointer): string {
-    const str = this.module.UTF8ToString(ptr)
-    this.module._free(ptr)
-    return str
+    try {
+      return this.module.UTF8ToString(ptr)
+    } finally {
+      this.module._free(ptr)
+    }
   }
 }

packages/quickjs-emscripten-core/src/runtime.ts

@@ -251,7 +251,7 @@ export class QuickJSRuntime extends UsingDisposable implements Disposable {
       ctxPtrOut.value.ptr,
     )
 
-    const ctxPtr = ctxPtrOut.value.typedArray[0] as JSContextPointer
+    const ctxPtr = ctxPtrOut.value.typedArray.value[0] as JSContextPointer
     ctxPtrOut.dispose()
     if (ctxPtr === 0) {
       // No jobs executed.

packages/quickjs-emscripten/src/quickjs.test.ts

@@ -1191,6 +1191,168 @@ export default "the default";
     })
   })
 
+  describe("WASM memory growth", () => {
+    // Growing the WASM heap replaces `module.HEAPU8.buffer` and detaches every
+    // TypedArray view created earlier. Reading a detached view yields `undefined`.
+    // These tests assert our out-parameter handling reads through the *current* heap.
+    const getModule = () =>
+      (vm as any).module as {
+        HEAPU8: Uint8Array
+        _malloc(n: number): number
+        _free(p: number): void
+      }
+
+    /** Force the heap to grow now. Throws if it didn't, so the tests below can't pass vacuously. */
+    function growHeap() {
+      const module = getModule()
+      const before = module.HEAPU8.buffer
+      // Blocks that fit in existing free space don't grow the heap, so double the size until one doesn't.
+      // (Compare with `!==`: assert.notStrictEqual would try to print the whole heap on failure.)
+      for (let mb = 16; module.HEAPU8.buffer === before; mb *= 2) {
+        assert.ok(mb <= 512, "heap did not grow")
+        module._free(module._malloc(mb * 1024 * 1024))
+      }
+    }
+
+    it("newTypedArray reads current heap contents after growth", () => {
+      const memory = (vm as any).memory as any
+      using array = memory.newMutablePointerArray(2)
+      const staleView = array.value.typedArray.value
+      growHeap()
+      assert.equal(staleView.length, 0, "precondition: the old view is detached")
+
+      const module = getModule()
+      new Int32Array(module.HEAPU8.buffer, array.value.ptr, 2).set([1234, 5678])
+      assert.deepEqual(Array.from(array.value.typedArray.value), [1234, 5678])
+    })
+
+    it("newTypedArray zero-initializes", () => {
+      const memory = (vm as any).memory as any
+      // Dirty the heap first so zeroing is observable.
+      const module = getModule()
+      const dirty = module._malloc(64)
+      module.HEAPU8.fill(0xff, dirty, dirty + 64)
+      module._free(dirty)
+      using array = memory.newMutablePointerArray(8)
+      assert.deepEqual(Array.from(array.value.typedArray.value), new Array(8).fill(0))
+    })
+
+    it(".getLength() after the heap grew since the context was created", () => {
+      growHeap()
+      using array = vm.unwrapResult(vm.evalCode(`new Array(1234).fill(0)`))
+      assert.strictEqual(vm.getLength(array), 1234)
+    })
+
+    it(".getOwnPropertyNames() after the heap grew, with many keys", () => {
+      growHeap()
+      const count = 50_000
+      using object = vm.unwrapResult(
+        vm.evalCode(`const o = {}; for (let i = 0; i < ${count}; i++) o["k" + i] = i; o`),
+      )
+      using names = vm.unwrapResult(vm.getOwnPropertyNames(object))
+      assert.strictEqual(names.length, count)
+      assert.strictEqual(vm.getString(names[0]), "k0")
+      assert.strictEqual(vm.getString(names[count - 1]), `k${count - 1}`)
+    })
+
+    it(".getArrayBuffer() result is still readable after the heap grew", () => {
+      using handle = vm.unwrapResult(vm.evalCode(`new Uint8Array([1, 2, 3, 4]).buffer`))
+      using bytes = vm.getArrayBuffer(handle)
+      growHeap()
+      assert.deepEqual(Array.from(bytes.value), [1, 2, 3, 4])
+    })
+
+    it(".executePendingJobs() when a job grows the heap", () => {
+      const module = getModule()
+      const bufferBefore = module.HEAPU8.buffer
+      // The heap may already be large from earlier tests, so the job allocates until the host
+      // sees the heap grow, rather than assuming a fixed amount is enough.
+      using heapGrew = vm.newFunction("heapGrew", () =>
+        vm.newNumber(module.HEAPU8.buffer !== bufferBefore ? 1 : 0),
+      )
+      vm.setProp(vm.global, "heapGrew", heapGrew)
+      vm.unwrapResult(
+        vm.evalCode(`Promise.resolve().then(() => {
+          const kept = []
+          for (let n = 0; n < 1000; n++) {
+            kept.push(new ArrayBuffer(4 * 1024 * 1024))
+            if (heapGrew()) return
+          }
+          throw new Error("heap never grew")
+        })`),
+      ).dispose()
+      const result = vm.runtime.executePendingJobs()
+      assert.ok(module.HEAPU8.buffer !== bufferBefore, "precondition: the job grew the heap")
+      // Throws if the job threw (e.g. "heap never grew").
+      assert.strictEqual(vm.unwrapResult(result), 1)
+      // The job ran in `vm`; a stale ctxPtr would have created a second, leaked context.
+      assert.strictEqual((vm.runtime as any).contextMap.size, 1)
+    })
+  })
+
+  describe("failure paths free host memory", () => {
+    /** Record every pointer malloc'd and freed while `fn` runs. */
+    function trackHeap<T>(fn: () => T): {
+      result: T | undefined
+      error: unknown
+      leaked: number[]
+    } {
+      const module = (vm as any).module
+      const { _malloc, _free } = module
+      const live = new Set<number>()
+      module._malloc = (n: number) => {
+        const ptr = _malloc(n)
+        live.add(ptr)
+        return ptr
+      }
+      module._free = (ptr: number) => {
+        live.delete(ptr)
+        return _free(ptr)
+      }
+      try {
+        return { result: fn(), error: undefined, leaked: [...live] }
+      } catch (error) {
+        return { result: undefined, error, leaked: [...live] }
+      } finally {
+        module._malloc = _malloc
+        module._free = _free
+        live.forEach((ptr) => _free(ptr))
+      }
+    }
+
+    it(".getString() frees the C string if decoding throws", () => {
+      using handle = vm.newString("hello")
+      const module = (vm as any).module
+      const UTF8ToString = module.UTF8ToString
+      const ffi = (vm as any).ffi
+      const QTS_FreeCString = ffi.QTS_FreeCString
+      let freed = 0
+      ffi.QTS_FreeCString = (...args: unknown[]) => {
+        freed++
+        return QTS_FreeCString.apply(ffi, args)
+      }
+      module.UTF8ToString = () => {
+        throw new Error("decode failed")
+      }
+      try {
+        assert.throws(() => vm.getString(handle), /decode failed/)
+      } finally {
+        module.UTF8ToString = UTF8ToString
+        ffi.QTS_FreeCString = QTS_FreeCString
+      }
+      assert.strictEqual(freed, 1)
+    })
+
+    it(".newArrayBuffer() frees the host buffer and throws if QuickJS cannot create it", () => {
+      // With no headroom, allocating the ArrayBuffer object fails.
+      vm.runtime.setMemoryLimit(1)
+      const { error, leaked } = trackHeap(() => vm.newArrayBuffer(new Uint8Array([1, 2, 3]).buffer))
+      vm.runtime.setMemoryLimit(-1)
+      assert.ok(error, "expected newArrayBuffer to throw")
+      assert.deepEqual(leaked, [])
+    })
+  })
+
   describe("memory pressure", () => {
     it("can pass a large string to a C function", async () => {
       const jsonString = await fs.promises.readFile(