From 8577f9ca852db7526df223827598573aa5937cff Mon Sep 17 00:00:00 2001 From: Tim Stableford Date: Fri, 8 Dec 2023 12:09:19 +0000 Subject: [PATCH] Various safety changes, bugfixes and added null type * Support timing out callbacks from JS into Lua * Made calling functions from JS into Lua more robust. Previously there was a failure cause where if you created a thread, had that thread return a function, close the thread and attempt to call the function it would try to call the Lua function from within the context of a closed thread. All function calls from JS into Lua now use there own individual calling thread as Lua does when calling C functions and those threads are within a thread only used for functions within the Lua reference registry. This had an interesting side-effect that setInterval blocked the tests from completing because it no longer silently errored. * Inject null type * Protect against unable to allocate memory for thread creation * Fix thread missing parent reference in some cases --- README.md | 2 +- src/engine.ts | 17 +++++- src/global.ts | 7 ++- src/thread.ts | 4 +- src/type-extensions/function.ts | 86 +++++++++++++++++++------- src/type-extensions/null.ts | 78 ++++++++++++++++++++++++ test/engine.test.js | 104 +++++++++++++++++++++++++++++--- 7 files changed, 261 insertions(+), 37 deletions(-) create mode 100644 src/type-extensions/null.ts diff --git a/README.md b/README.md index a519ed5..2826b3e 100644 --- a/README.md +++ b/README.md @@ -186,7 +186,7 @@ npm test # ensure everything it's working fine ### Null -`null` is not exposed to Lua and it has no awareness of it which can cause some issues when using it a table. `nil` is equivalent to `undefined`. Issue #39 tracks this and a workaround until `null` is added into Wasmoon. +`null` is injected as userdata type if `injectObjects` is set to `true`. This works as expected except that it will evaluate to `true` in Lua. ### Promises diff --git a/src/engine.ts b/src/engine.ts index a3f2511..f8875e2 100755 --- a/src/engine.ts +++ b/src/engine.ts @@ -2,6 +2,7 @@ import Global from './global' import Thread from './thread' import createErrorType from './type-extensions/error' import createFunctionType from './type-extensions/function' +import createNullType from './type-extensions/null' import createPromiseType from './type-extensions/promise' import createProxyType from './type-extensions/proxy' import createTableType from './type-extensions/table' @@ -13,17 +14,29 @@ export default class LuaEngine { public constructor( private cmodule: LuaWasm, - { openStandardLibs = true, injectObjects = false, enableProxy = true, traceAllocations = false } = {}, + { + openStandardLibs = true, + injectObjects = false, + enableProxy = true, + traceAllocations = false, + functionTimeout = undefined as number | undefined, + } = {}, ) { this.global = new Global(this.cmodule, traceAllocations) // Generic handlers - These may be required to be registered for additional types. this.global.registerTypeExtension(0, createTableType(this.global)) - this.global.registerTypeExtension(0, createFunctionType(this.global)) + this.global.registerTypeExtension(0, createFunctionType(this.global, { functionTimeout })) // Contains the :await functionality. this.global.registerTypeExtension(1, createPromiseType(this.global, injectObjects)) + if (injectObjects) { + // Should be higher priority than table since that catches generic objects along + // with userdata so it doesn't end up a userdata type. + this.global.registerTypeExtension(5, createNullType(this.global)) + } + if (enableProxy) { // This extension only really overrides tables and arrays. // When a function is looked up in one of it's tables it's bound and then diff --git a/src/global.ts b/src/global.ts index ce20728..d0cbfce 100755 --- a/src/global.ts +++ b/src/global.ts @@ -41,7 +41,12 @@ export default class Global extends Thread { 'iiiii', ) - super(cmodule, [], cmodule.lua_newstate(allocatorFunctionPointer, null)) + const address = cmodule.lua_newstate(allocatorFunctionPointer, null) + if (!address) { + cmodule.module.removeFunction(allocatorFunctionPointer) + throw new Error('lua_newstate returned a null pointer') + } + super(cmodule, [], address) this.memoryStats = memoryStats this.allocatorFunctionPointer = allocatorFunctionPointer diff --git a/src/thread.ts b/src/thread.ts index bc82245..12e26f4 100755 --- a/src/thread.ts +++ b/src/thread.ts @@ -45,7 +45,7 @@ export default class Thread { if (!address) { throw new Error('lua_newthread returned a null pointer') } - return new Thread(this.lua, this.typeExtensions, address) + return new Thread(this.lua, this.typeExtensions, address, this.parent || this) } public resetThread(): void { @@ -186,7 +186,7 @@ export default class Thread { public pushValue(rawValue: unknown, userdata?: unknown): void { const decoratedValue = this.getValueDecorations(rawValue) - const target = decoratedValue.target ?? undefined + const target = decoratedValue.target if (target instanceof Thread) { const isMain = this.lua.lua_pushthread(target.address) === 1 diff --git a/src/type-extensions/function.ts b/src/type-extensions/function.ts index d4875cd..ecab19c 100644 --- a/src/type-extensions/function.ts +++ b/src/type-extensions/function.ts @@ -18,6 +18,10 @@ export function decorateFunction(target: FunctionType, options: FunctionDecorati return new Decoration(target, options) } +export interface FunctionTypeExtensionOptions { + functionTimeout?: number +} + class FunctionTypeExtension extends TypeExtension { private readonly functionRegistry = typeof FinalizationRegistry !== 'undefined' @@ -30,10 +34,21 @@ class FunctionTypeExtension extends TypeExtension { - if (thread.isClosed()) { + // Calling a function would ideally be in the Lua context that's calling it. For example if the JS function + // setInterval were exposed to Lua then the calling thread would be created in that Lua context for executing + // the function call back to Lua through JS. However, if getValue were called in a thread, the thread then + // destroyed, and then this JS func were called it would be calling from a dead context. That means the safest + // thing to do is to have a thread you know will always exist. + if (this.callbackContext.isClosed()) { console.warn('Tried to call a function after closing lua state') return } - const internalType = thread.lua.lua_rawgeti(thread.address, LUA_REGISTRYINDEX, BigInt(func)) - if (internalType !== LuaType.Function) { - const callMetafieldType = thread.lua.luaL_getmetafield(thread.address, -1, '__call') - thread.pop() - if (callMetafieldType !== LuaType.Function) { - throw new Error(`A value of type '${internalType}' was pushed but it is not callable`) + // Function calls back to value should always be within a new thread because + // they can be left in inconsistent states. + const callThread = this.callbackContext.newThread() + try { + const internalType = callThread.lua.lua_rawgeti(callThread.address, LUA_REGISTRYINDEX, BigInt(func)) + if (internalType !== LuaType.Function) { + const callMetafieldType = callThread.lua.luaL_getmetafield(callThread.address, -1, '__call') + callThread.pop() + if (callMetafieldType !== LuaType.Function) { + throw new Error(`A value of type '${internalType}' was pushed but it is not callable`) + } } - } - for (const arg of args) { - thread.pushValue(arg) - } + for (const arg of args) { + callThread.pushValue(arg) + } - const status: LuaReturn = thread.lua.lua_pcallk(thread.address, args.length, 1, 0, 0, null) - if (status === LuaReturn.Yield) { - throw new Error('cannot yield in callbacks from javascript') - } - thread.assertOk(status) + if (this.options?.functionTimeout) { + callThread.setTimeout(Date.now() + this.options.functionTimeout) + } - const result = thread.getValue(-1) + const status: LuaReturn = callThread.lua.lua_pcallk(callThread.address, args.length, 1, 0, 0, null) + if (status === LuaReturn.Yield) { + throw new Error('cannot yield in callbacks from javascript') + } + callThread.assertOk(status) - thread.pop() - return result + if (callThread.getTop() > 0) { + return callThread.getValue(-1) + } + return undefined + } finally { + callThread.close() + // Pop thread used for function call. + this.callbackContext.pop() + } } this.functionRegistry?.register(jsFunc, func) @@ -195,6 +234,9 @@ class FunctionTypeExtension extends TypeExtension { - return new FunctionTypeExtension(thread) +export default function createTypeExtension( + thread: Global, + options?: FunctionTypeExtensionOptions, +): TypeExtension { + return new FunctionTypeExtension(thread, options) } diff --git a/src/type-extensions/null.ts b/src/type-extensions/null.ts new file mode 100644 index 0000000..7f95a73 --- /dev/null +++ b/src/type-extensions/null.ts @@ -0,0 +1,78 @@ +import { Decoration } from '../decoration' +import { LuaReturn, LuaState } from '../types' +import Global from '../global' +import Thread from '../thread' +import TypeExtension from '../type-extension' + +class NullTypeExtension extends TypeExtension { + private gcPointer: number + + public constructor(thread: Global) { + super(thread, 'js_null') + + this.gcPointer = thread.lua.module.addFunction((functionStateAddress: LuaState) => { + // Throws a lua error which does a jump if it does not match. + const userDataPointer = thread.lua.luaL_checkudata(functionStateAddress, 1, this.name) + const referencePointer = thread.lua.module.getValue(userDataPointer, '*') + thread.lua.unref(referencePointer) + + return LuaReturn.Ok + }, 'ii') + + if (thread.lua.luaL_newmetatable(thread.address, this.name)) { + const metatableIndex = thread.lua.lua_gettop(thread.address) + + // Mark it as uneditable + thread.lua.lua_pushstring(thread.address, 'protected metatable') + thread.lua.lua_setfield(thread.address, metatableIndex, '__metatable') + + // Add the gc function + thread.lua.lua_pushcclosure(thread.address, this.gcPointer, 0) + thread.lua.lua_setfield(thread.address, metatableIndex, '__gc') + + // Add an __index method that returns nothing. + thread.pushValue(() => null) + thread.lua.lua_setfield(thread.address, metatableIndex, '__index') + + thread.pushValue(() => 'null') + thread.lua.lua_setfield(thread.address, metatableIndex, '__tostring') + + thread.pushValue((self: unknown, other: unknown) => self === other) + thread.lua.lua_setfield(thread.address, metatableIndex, '__eq') + } + // Pop the metatable from the stack. + thread.lua.lua_pop(thread.address, 1) + + // Create a new table, this is unique and will be the "null" value by attaching the + // metatable created above. The first argument is the target, the second options. + super.pushValue(thread, new Decoration({}, {})) + // Put it into the global field named null. + thread.lua.lua_setglobal(thread.address, 'null') + } + + public getValue(thread: Thread, index: number): null { + const refUserData = thread.lua.luaL_testudata(thread.address, index, this.name) + if (!refUserData) { + throw new Error(`data does not have the expected metatable: ${this.name}`) + } + return null + } + + // any because LuaDecoration is not exported from the Lua lib. + public pushValue(thread: Thread, decoration: any): boolean { + if (decoration?.target !== null) { + return false + } + // Rather than pushing a new value, get the global "null" onto the stack. + thread.lua.lua_getglobal(thread.address, 'null') + return true + } + + public close(): void { + this.thread.lua.module.removeFunction(this.gcPointer) + } +} + +export default function createTypeExtension(thread: Global): TypeExtension { + return new NullTypeExtension(thread) +} diff --git a/test/engine.test.js b/test/engine.test.js index 719ffa1..7677358 100755 --- a/test/engine.test.js +++ b/test/engine.test.js @@ -2,6 +2,7 @@ const { LuaLibraries, LuaReturn, LuaThread, LuaType, decorate, decorateProxy, de const { expect } = require('chai') const { getEngine, getFactory } = require('./utils') const { setTimeout } = require('node:timers/promises') +const { EventEmitter } = require('events') const jestMock = require('jest-mock') class TestClass { @@ -19,6 +20,18 @@ class TestClass { } describe('Engine', () => { + let intervals = [] + const setIntervalSafe = (callback, interval) => { + intervals.push(setInterval(() => callback(), interval)) + } + + afterEach(() => { + for (const interval of intervals) { + clearInterval(interval) + } + intervals = [] + }) + it('receive lua table on JS function should succeed', async () => { const engine = await getEngine() engine.global.set('stringify', (table) => { @@ -170,7 +183,7 @@ describe('Engine', () => { it('scheduled lua calls should succeed', async () => { const engine = await getEngine() - engine.global.set('setInterval', setInterval) + engine.global.set('setInterval', setIntervalSafe) await engine.doString(` test = "" @@ -188,7 +201,7 @@ describe('Engine', () => { it('scheduled lua calls should fail silently if invalid', async () => { const engine = await getEngine() - engine.global.set('setInterval', setInterval) + engine.global.set('setInterval', setIntervalSafe) // TODO: Disable mock at the end of the test. jestMock.spyOn(console, 'warn').mockImplementation(() => { @@ -497,14 +510,12 @@ describe('Engine', () => { it('timeout blocking lua program', async () => { const engine = await getEngine() - const thread = engine.global.newThread() - - thread.loadString(` - local i = 0 - while true do i = i + 1 end - `) + engine.global.loadString(` + local i = 0 + while true do i = i + 1 end + `) - await expect(thread.run(0, { timeout: 5 })).eventually.to.be.rejectedWith('thread timeout exceeded') + await expect(engine.global.run(0, { timeout: 5 })).eventually.to.be.rejectedWith('thread timeout exceeded') }) it('overwrite lib function', async () => { @@ -708,4 +719,79 @@ describe('Engine', () => { expect(res).to.be.equal('1689031554550') }) + + it('yielding in a JS callback into Lua does not break lua state', async () => { + // When yielding within a callback the error 'attempt to yield across a C-call boundary'. + // This test just checks that throwing that error still allows the lua global to be + // re-used and doesn't cause JS to abort or some nonsense. + const engine = await getEngine() + const testEmitter = new EventEmitter() + engine.global.set('yield', () => new Promise((resolve) => testEmitter.once('resolve', resolve))) + const resPromise = engine.doString(` + local res = yield():next(function () + coroutine.yield() + return 15 + end) + print("res", res:await()) + `) + + testEmitter.emit('resolve') + await expect(resPromise).to.eventually.be.rejectedWith('Error: attempt to yield across a C-call boundary') + + expect(await engine.doString(`return 42`)).to.equal(42) + }) + + it('forced yield within JS callback from Lua doesnt cause vm to crash', async () => { + const engine = await getEngine({ functionTimeout: 10 }) + engine.global.set('promise', Promise.resolve()) + const thread = engine.global.newThread() + thread.loadString(` + promise:next(function () + while true do + -- nothing + end + end):await() + `) + await expect(thread.run(0, { timeout: 5 })).to.eventually.be.rejectedWith('thread timeout exceeded') + + expect(await engine.doString(`return 42`)).to.equal(42) + }) + + it('function callback timeout still allows timeout of caller thread', async () => { + const engine = await getEngine() + engine.global.set('promise', Promise.resolve()) + const thread = engine.global.newThread() + thread.loadString(` + promise:next(function () + -- nothing + end):await() + while true do end + `) + await expect(thread.run(0, { timeout: 5 })).to.eventually.be.rejectedWith('thread timeout exceeded') + }) + + it('null injected and valid', async () => { + const engine = await getEngine() + engine.global.loadString(` + local args = { ... } + assert(args[1] == null, string.format("expected first argument to be null, got %s", tostring(args[1]))) + return null, args[1], tostring(null) + `) + engine.global.pushValue(null) + const res = await engine.global.run(1) + expect(res).to.deep.equal([null, null, 'null']) + }) + + it('Nested callback from JS to Lua', async () => { + const engine = await getEngine() + engine.global.set('call', (fn) => fn()) + const res = await engine.doString(` + return call(function () + return call(function () + return 10 + end) + end) + `) + expect(res).to.equal(10) + }) })