Fix for func_id rollover (justjake#94)

* 32767 functions good, 32768 functions BAD * change `magic` to uin16_t (avoids signed intereger overflow) * type magic as uint32_t, add simple test * re-enable all tests * remove missed test code * address PR issues * switch to a map of maps for fnMap * update fnId to start at min value * skip max funcID tests for debug mode * missed a flag * run prettier
decentraland · May 3, 2023 · abc75a2 · abc75a2
1 parent e43b888
commit abc75a2
Show file tree

Hide file tree

Showing 7 changed files with 72 additions and 19 deletions.
diff --git a/c/interface.c b/c/interface.c
@@ -567,7 +567,7 @@ int QTS_BuildIsAsyncify() {
 // -------------------
 // function: C -> Host
 #ifdef __EMSCRIPTEN__
-EM_JS(MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, int magic_func_id), {
+EM_JS(MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, uint32_t magic_func_id), {
 #ifdef QTS_ASYNCIFY
   const asyncify = {['handleSleep'] : Asyncify.handleSleep};
 #else
@@ -578,7 +578,7 @@ EM_JS(MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueCo
 #endif
 
 // Function: QuickJS -> C
-JSValue qts_call_function(JSContext *ctx, JSValueConst this_val, int argc, JSValueConst *argv, int magic) {
+JSValue qts_call_function(JSContext *ctx, JSValueConst this_val, int argc, JSValueConst *argv, uint32_t magic) {
   JSValue *result_ptr = qts_host_call_function(ctx, &this_val, argc, argv, magic);
   if (result_ptr == NULL) {
     return JS_UNDEFINED;
@@ -589,7 +589,7 @@ JSValue qts_call_function(JSContext *ctx, JSValueConst this_val, int argc, JSVal
 }
 
 // Function: Host -> QuickJS
-JSValue *QTS_NewFunction(JSContext *ctx, int func_id, const char *name) {
+JSValue *QTS_NewFunction(JSContext *ctx, uint32_t func_id, const char *name) {
 #ifdef QTS_DEBUG_MODE
   char msg[500];
   sprintf(msg, "new_function(name: %s, magic: %d)", name, func_id);

diff --git a/generate.ts b/generate.ts
@@ -135,7 +135,13 @@ function cTypeToTypescriptType(ctype: string): ParsedType {
   if (type === "void") {
     ffi = null
   }
-  if (type === "double" || type === "int" || type === "size_t") {
+  if (
+    type === "double" ||
+    type === "int" ||
+    type === "size_t" ||
+    type === "uint16_t" ||
+    type === "uint32_t"
+  ) {
     ffi = "number"
     typescript = "number"
   }
@@ -237,8 +243,8 @@ function buildSyncSymbols(matches: RegExpMatchArray[]) {
   return filtered.map((fn) => "_" + fn.functionName)
 }
 
-// Input: EM_JS(MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, int magic_func_id), {
-// Match: MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, int magic_func_id)
+// Input: EM_JS(MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, uint32_t magic_func_id), {
+// Match: MaybeAsync(JSValue *), qts_host_call_function, (JSContext * ctx, JSValueConst *this_ptr, int argc, JSValueConst *argv, uint32_t magic_func_id)
 function buildAsyncifySymbols(matches: RegExpMatchArray[]) {
   const parsed = matches.map((match) => {
     const [, contents] = match
@@ -312,7 +318,7 @@ export function matchAll(regexp: RegExp, text: string) {
   // We're using .exec, which mutates the regexp by setting the .lastIndex
   const initialLastIndex = regexp.lastIndex
   const result: RegExpExecArray[] = []
-  let match = null
+  let match: RegExpExecArray | null = null
   while ((match = regexp.exec(text)) !== null) {
     result.push(match)
   }

diff --git a/ts/context.ts b/ts/context.ts
@@ -412,7 +412,7 @@ export class QuickJSContext implements LowLevelJavascriptVm<QuickJSHandle>, Disp
    */
   newFunction(name: string, fn: VmFunctionImplementation<QuickJSHandle>): QuickJSHandle {
     const fnId = ++this.fnNextId
-    this.fnMap.set(fnId, fn)
+    this.setFunction(fnId, fn)
     return this.memory.heapValueHandle(this.ffi.QTS_NewFunction(this.ctx.value, fnId, name))
   }
 
@@ -784,9 +784,30 @@ export class QuickJSContext implements LowLevelJavascriptVm<QuickJSHandle>, Disp
   }
 
   /** @private */
-  protected fnNextId = 0
+  protected fnNextId = -32768 // min value of signed 16bit int used by Quickjs
   /** @private */
-  protected fnMap = new Map<number, VmFunctionImplementation<QuickJSHandle>>()
+  protected fnMaps = new Map<number, Map<number, VmFunctionImplementation<QuickJSHandle>>>()
+
+  /** @private */
+  protected getFunction(fn_id: number): VmFunctionImplementation<QuickJSHandle> | undefined {
+    const map_id = fn_id >> 8
+    const fnMap = this.fnMaps.get(map_id)
+    if (!fnMap) {
+      return undefined
+    }
+    return fnMap.get(fn_id)
+  }
+
+  /** @private */
+  protected setFunction(fn_id: number, handle: VmFunctionImplementation<QuickJSHandle>) {
+    const map_id = fn_id >> 8
+    let fnMap = this.fnMaps.get(map_id)
+    if (!fnMap) {
+      fnMap = new Map<number, VmFunctionImplementation<QuickJSHandle>>()
+      this.fnMaps.set(map_id, fnMap)
+    }
+    return fnMap.set(fn_id, handle)
+  }
 
   /**
    * @hidden
@@ -797,8 +818,9 @@ export class QuickJSContext implements LowLevelJavascriptVm<QuickJSHandle>, Disp
         throw new Error("QuickJSContext instance received C -> JS call with mismatched ctx")
       }
 
-      const fn = this.fnMap.get(fn_id)
+      const fn = this.getFunction(fn_id)
       if (!fn) {
+        // this "throw" is not catch-able from the TS side. could we somehow handle this higher up?
         throw new Error(`QuickJSContext had no callback with id ${fn_id}`)
       }
 

diff --git a/ts/generated/emscripten-module.WASM_DEBUG_ASYNCIFY.wasm.map b/ts/generated/emscripten-module.WASM_DEBUG_ASYNCIFY.wasm.map
diff --git a/ts/generated/emscripten-module.WASM_DEBUG_SYNC.js b/ts/generated/emscripten-module.WASM_DEBUG_SYNC.js
@@ -2371,8 +2371,8 @@ var tempI64;
 // === Body ===
 
 var ASM_CONSTS = {
-  118990: function() {return withBuiltinMalloc(function () { return allocateUTF8(Module['LSAN_OPTIONS'] || 0); });},  
- 119087: function() {var setting = Module['printWithColors']; if (setting != null) { return setting; } else { return ENVIRONMENT_IS_NODE && process.stderr.isTTY; }}
+  113795: function() {return withBuiltinMalloc(function () { return allocateUTF8(Module['LSAN_OPTIONS'] || 0); });},  
+ 113892: function() {var setting = Module['printWithColors']; if (setting != null) { return setting; } else { return ENVIRONMENT_IS_NODE && process.stderr.isTTY; }}
 };
 function qts_host_call_function(ctx,this_ptr,argc,argv,magic_func_id){ const asyncify = undefined; return Module['callbacks']['callFunction'](asyncify, ctx, this_ptr, argc, argv, magic_func_id); }
 function qts_host_interrupt_handler(rt){ const asyncify = undefined; return Module['callbacks']['shouldInterrupt'](asyncify, rt); }

diff --git a/ts/generated/emscripten-module.WASM_RELEASE_ASYNCIFY.js b/ts/generated/emscripten-module.WASM_RELEASE_ASYNCIFY.js
diff --git a/ts/quickjs.test.ts b/ts/quickjs.test.ts
@@ -13,7 +13,7 @@ import {
 import { it, describe } from "mocha"
 import assert from "assert"
 import { isFail, VmCallResult } from "./vm-interface"
-import fs from "fs"
+import fs, { chmod } from "fs"
 import { QuickJSContext } from "./context"
 import { QuickJSAsyncContext } from "./context-asyncify"
 import { DEBUG_ASYNC, DEBUG_SYNC, memoizePromiseFactory, QuickJSFFI } from "./variants"
@@ -23,7 +23,7 @@ import { EitherFFI } from "./types"
 
 const TEST_NO_ASYNC = Boolean(process.env.TEST_NO_ASYNC)
 
-function contextTests(getContext: () => Promise<QuickJSContext>) {
+function contextTests(getContext: () => Promise<QuickJSContext>, isDebug = false) {
   let vm: QuickJSContext = undefined as any
   let ffi: EitherFFI = undefined as any
   let testId = 0
@@ -150,6 +150,31 @@ function contextTests(getContext: () => Promise<QuickJSContext>) {
 
       fnHandle.dispose()
     })
+
+    it("can handle more than signed int max functions being registered", function (done) {
+      // test for unsigned func_id impl
+      this.timeout(30000) // we need more time to register 2^16 functions
+
+      if (isDebug) {
+        this.skip() // no need to run this again, and it takes WAY too long
+      }
+
+      for (let i = 0; i < Math.pow(2, 16); i++) {
+        const funcID = i
+        const fnHandle = vm.newFunction(`__func-${i}`, () => {
+          return vm.newNumber(funcID)
+        })
+        if (i % 1024 === 0) {
+          // spot check every 1024 funcs
+          const res = vm.unwrapResult(vm.callFunction(fnHandle, vm.undefined))
+          const calledFuncID = vm.dump(res)
+          assert(calledFuncID === i)
+          res.dispose()
+        }
+        fnHandle.dispose()
+      }
+      done()
+    })
   })
 
   describe("properties", () => {
@@ -921,7 +946,7 @@ describe("QuickJSContext", function () {
   describe("DEBUG sync module", function () {
     const loader = memoizePromiseFactory(() => newQuickJSWASMModule(DEBUG_SYNC))
     const getContext = () => loader().then((mod) => mod.newContext())
-    contextTests.call(this, getContext)
+    contextTests.call(this, getContext, true)
   })
 })
 
@@ -945,7 +970,7 @@ if (!TEST_NO_ASYNC) {
       const getContext = () => loader().then((mod) => mod.newContext())
 
       describe("sync API", () => {
-        contextTests(getContext)
+        contextTests(getContext, true)
       })
 
       describe("async API", () => {