From d2556b402ad9b5897d181af9594351af61d08b28 Mon Sep 17 00:00:00 2001 From: Yuta Saito Date: Tue, 17 May 2022 01:08:24 +0900 Subject: [PATCH] Improve JSKit diagnostics for use-after-free of JSClosure To mitigate the debugging difficulties, JSKit should provide more information in diagnostics. --- .../Sources/PrimaryTests/UnitTestUtils.swift | 6 ++++++ .../TestSuites/Sources/PrimaryTests/main.swift | 11 +++++++++++ Runtime/src/index.ts | 14 +++++++++----- Runtime/src/types.ts | 4 ++-- .../FundamentalObjects/JSClosure.swift | 18 ++++++++++++------ Sources/JavaScriptKit/Runtime/index.js | 14 +++++++++----- Sources/JavaScriptKit/Runtime/index.mjs | 14 +++++++++----- Sources/JavaScriptKit/XcodeSupport.swift | 2 +- Sources/_CJavaScriptKit/_CJavaScriptKit.c | 9 +++++---- .../_CJavaScriptKit/include/_CJavaScriptKit.h | 5 ++++- 10 files changed, 68 insertions(+), 29 deletions(-) diff --git a/IntegrationTests/TestSuites/Sources/PrimaryTests/UnitTestUtils.swift b/IntegrationTests/TestSuites/Sources/PrimaryTests/UnitTestUtils.swift index ab11bc017..571e0d6a3 100644 --- a/IntegrationTests/TestSuites/Sources/PrimaryTests/UnitTestUtils.swift +++ b/IntegrationTests/TestSuites/Sources/PrimaryTests/UnitTestUtils.swift @@ -94,6 +94,12 @@ func expectString(_ value: JSValue, file: StaticString = #file, line: UInt = #li } } +func expect(_ description: String, _ result: Bool, file: StaticString = #file, line: UInt = #line, column: UInt = #column) throws { + if !result { + throw MessageError(description, file: file, line: line, column: column) + } +} + func expectThrow(_ body: @autoclosure () throws -> T, file: StaticString = #file, line: UInt = #line, column: UInt = #column) throws -> Error { do { _ = try body() diff --git a/IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift b/IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift index a48c6fb00..aede07ced 100644 --- a/IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift +++ b/IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift @@ -240,6 +240,17 @@ try test("Closure Lifetime") { // OneshotClosure won't call fatalError even if it's deallocated before `release` } #endif + +#if JAVASCRIPTKIT_WITHOUT_WEAKREFS + // Check diagnostics of use-after-free + do { + let c1 = JSClosure { $0[0] } + c1.release() + let error = try expectThrow(try evalClosure.throws(c1, JSValue.number(42.0))) as! JSValue + try expect("Error message should contains definition location", error.description.hasSuffix("PrimaryTests/main.swift:247")) + } +#endif + } try test("Host Function Registration") { diff --git a/Runtime/src/index.ts b/Runtime/src/index.ts index 17c53696e..3da0f2e47 100644 --- a/Runtime/src/index.ts +++ b/Runtime/src/index.ts @@ -14,7 +14,7 @@ export class SwiftRuntime { private _instance: WebAssembly.Instance | null; private _memory: Memory | null; private _closureDeallocator: SwiftClosureDeallocator | null; - private version: number = 707; + private version: number = 708; private textDecoder = new TextDecoder("utf-8"); private textEncoder = new TextEncoder(); // Only support utf-8 @@ -68,7 +68,7 @@ export class SwiftRuntime { return this._closureDeallocator; } - private callHostFunction(host_func_id: number, args: any[]) { + private callHostFunction(host_func_id: number, line: number, file: string, args: any[]) { const argc = args.length; const argv = this.exports.swjs_prepare_host_function_call(argc); for (let index = 0; index < args.length; index++) { @@ -88,12 +88,15 @@ export class SwiftRuntime { const callback_func_ref = this.memory.retain((result: any) => { output = result; }); - this.exports.swjs_call_host_function( + const alreadyReleased = this.exports.swjs_call_host_function( host_func_id, argv, argc, callback_func_ref ); + if (alreadyReleased) { + throw new Error(`The JSClosure has been already released by Swift side. The closure is created at ${file}:${line}`); + } this.exports.swjs_cleanup_host_function_call(argv); return output; } @@ -371,9 +374,10 @@ export class SwiftRuntime { return obj instanceof constructor; }, - swjs_create_function: (host_func_id: number) => { + swjs_create_function: (host_func_id: number, line: number, file: ref) => { + const fileString = this.memory.getObject(file) as string; const func = (...args: any[]) => - this.callHostFunction(host_func_id, args); + this.callHostFunction(host_func_id, line, fileString, args); const func_ref = this.memory.retain(func); this.closureDeallocator?.track(func, func_ref); return func_ref; diff --git a/Runtime/src/types.ts b/Runtime/src/types.ts index d587b2c8a..913837e32 100644 --- a/Runtime/src/types.ts +++ b/Runtime/src/types.ts @@ -14,7 +14,7 @@ export interface ExportedFunctions { argv: pointer, argc: number, callback_func_ref: ref - ): void; + ): bool; swjs_free_host_function(host_func_id: number): void; } @@ -95,7 +95,7 @@ export interface ImportedFunctions { exception_payload2_ptr: pointer ): number; swjs_instanceof(obj_ref: ref, constructor_ref: ref): boolean; - swjs_create_function(host_func_id: number): number; + swjs_create_function(host_func_id: number, line: number, file: ref): number; swjs_create_typed_array( constructor_ref: ref, elementsPtr: pointer, diff --git a/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift b/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift index cbd44bd6e..48c4f9e82 100644 --- a/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift +++ b/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift @@ -15,13 +15,15 @@ public protocol JSClosureProtocol: JSValueCompatible { public class JSOneshotClosure: JSObject, JSClosureProtocol { private var hostFuncRef: JavaScriptHostFuncRef = 0 - public init(_ body: @escaping ([JSValue]) -> JSValue) { + public init(_ body: @escaping ([JSValue]) -> JSValue, file: String = #fileID, line: UInt32 = #line) { // 1. Fill `id` as zero at first to access `self` to get `ObjectIdentifier`. super.init(id: 0) // 2. Create a new JavaScript function which calls the given Swift function. hostFuncRef = JavaScriptHostFuncRef(bitPattern: Int32(ObjectIdentifier(self).hashValue)) - id = _create_function(hostFuncRef) + id = withExtendedLifetime(JSString(file)) { file in + _create_function(hostFuncRef, line, file.asInternalJSRef()) + } // 3. Retain the given body in static storage by `funcRef`. JSClosure.sharedClosures[hostFuncRef] = (self, { @@ -72,13 +74,15 @@ public class JSClosure: JSObject, JSClosureProtocol { }) } - public init(_ body: @escaping ([JSValue]) -> JSValue) { + public init(_ body: @escaping ([JSValue]) -> JSValue, file: String = #fileID, line: UInt32 = #line) { // 1. Fill `id` as zero at first to access `self` to get `ObjectIdentifier`. super.init(id: 0) // 2. Create a new JavaScript function which calls the given Swift function. hostFuncRef = JavaScriptHostFuncRef(bitPattern: Int32(ObjectIdentifier(self).hashValue)) - id = _create_function(hostFuncRef) + id = withExtendedLifetime(JSString(file)) { file in + _create_function(hostFuncRef, line, file.asInternalJSRef()) + } // 3. Retain the given body in static storage by `funcRef`. Self.sharedClosures[hostFuncRef] = (self, body) @@ -128,19 +132,21 @@ public class JSClosure: JSObject, JSClosureProtocol { // │ │ │ // └─────────────────────┴──────────────────────────┘ +/// Returns true if the host function has been already released, otherwise false. @_cdecl("_call_host_function_impl") func _call_host_function_impl( _ hostFuncRef: JavaScriptHostFuncRef, _ argv: UnsafePointer, _ argc: Int32, _ callbackFuncRef: JavaScriptObjectRef -) { +) -> Bool { guard let (_, hostFunc) = JSClosure.sharedClosures[hostFuncRef] else { - fatalError("The function was already released") + return true } let arguments = UnsafeBufferPointer(start: argv, count: Int(argc)).map(\.jsValue) let result = hostFunc(arguments) let callbackFuncRef = JSFunction(id: callbackFuncRef) _ = callbackFuncRef(result) + return false } diff --git a/Sources/JavaScriptKit/Runtime/index.js b/Sources/JavaScriptKit/Runtime/index.js index 68f1c433c..8b6edbe3f 100644 --- a/Sources/JavaScriptKit/Runtime/index.js +++ b/Sources/JavaScriptKit/Runtime/index.js @@ -190,7 +190,7 @@ class SwiftRuntime { constructor() { - this.version = 707; + this.version = 708; this.textDecoder = new TextDecoder("utf-8"); this.textEncoder = new TextEncoder(); // Only support utf-8 /** @deprecated Use `wasmImports` instead */ @@ -318,9 +318,10 @@ const constructor = this.memory.getObject(constructor_ref); return obj instanceof constructor; }, - swjs_create_function: (host_func_id) => { + swjs_create_function: (host_func_id, line, file) => { var _a; - const func = (...args) => this.callHostFunction(host_func_id, args); + const fileString = this.memory.getObject(file); + const func = (...args) => this.callHostFunction(host_func_id, line, fileString, args); const func_ref = this.memory.retain(func); (_a = this.closureDeallocator) === null || _a === void 0 ? void 0 : _a.track(func, func_ref); return func_ref; @@ -393,7 +394,7 @@ } return this._closureDeallocator; } - callHostFunction(host_func_id, args) { + callHostFunction(host_func_id, line, file, args) { const argc = args.length; const argv = this.exports.swjs_prepare_host_function_call(argc); for (let index = 0; index < args.length; index++) { @@ -406,7 +407,10 @@ const callback_func_ref = this.memory.retain((result) => { output = result; }); - this.exports.swjs_call_host_function(host_func_id, argv, argc, callback_func_ref); + const alreadyReleased = this.exports.swjs_call_host_function(host_func_id, argv, argc, callback_func_ref); + if (alreadyReleased) { + throw new Error(`The JSClosure has been already released by Swift side. The closure is created at ${file}:${line}`); + } this.exports.swjs_cleanup_host_function_call(argv); return output; } diff --git a/Sources/JavaScriptKit/Runtime/index.mjs b/Sources/JavaScriptKit/Runtime/index.mjs index 1874a8fee..465752eb5 100644 --- a/Sources/JavaScriptKit/Runtime/index.mjs +++ b/Sources/JavaScriptKit/Runtime/index.mjs @@ -184,7 +184,7 @@ class Memory { class SwiftRuntime { constructor() { - this.version = 707; + this.version = 708; this.textDecoder = new TextDecoder("utf-8"); this.textEncoder = new TextEncoder(); // Only support utf-8 /** @deprecated Use `wasmImports` instead */ @@ -312,9 +312,10 @@ class SwiftRuntime { const constructor = this.memory.getObject(constructor_ref); return obj instanceof constructor; }, - swjs_create_function: (host_func_id) => { + swjs_create_function: (host_func_id, line, file) => { var _a; - const func = (...args) => this.callHostFunction(host_func_id, args); + const fileString = this.memory.getObject(file); + const func = (...args) => this.callHostFunction(host_func_id, line, fileString, args); const func_ref = this.memory.retain(func); (_a = this.closureDeallocator) === null || _a === void 0 ? void 0 : _a.track(func, func_ref); return func_ref; @@ -387,7 +388,7 @@ class SwiftRuntime { } return this._closureDeallocator; } - callHostFunction(host_func_id, args) { + callHostFunction(host_func_id, line, file, args) { const argc = args.length; const argv = this.exports.swjs_prepare_host_function_call(argc); for (let index = 0; index < args.length; index++) { @@ -400,7 +401,10 @@ class SwiftRuntime { const callback_func_ref = this.memory.retain((result) => { output = result; }); - this.exports.swjs_call_host_function(host_func_id, argv, argc, callback_func_ref); + const alreadyReleased = this.exports.swjs_call_host_function(host_func_id, argv, argc, callback_func_ref); + if (alreadyReleased) { + throw new Error(`The JSClosure has been already released by Swift side. The closure is created at ${file}:${line}`); + } this.exports.swjs_cleanup_host_function_call(argv); return output; } diff --git a/Sources/JavaScriptKit/XcodeSupport.swift b/Sources/JavaScriptKit/XcodeSupport.swift index 4cde273f3..a3c8aeb1a 100644 --- a/Sources/JavaScriptKit/XcodeSupport.swift +++ b/Sources/JavaScriptKit/XcodeSupport.swift @@ -91,7 +91,7 @@ import _CJavaScriptKit _: JavaScriptObjectRef, _: JavaScriptObjectRef ) -> Bool { fatalError() } - func _create_function(_: JavaScriptHostFuncRef) -> JavaScriptObjectRef { fatalError() } + func _create_function(_: JavaScriptHostFuncRef, _: UInt32, _: JavaScriptObjectRef) -> JavaScriptObjectRef { fatalError() } func _create_typed_array( _: JavaScriptObjectRef, _: UnsafePointer, diff --git a/Sources/_CJavaScriptKit/_CJavaScriptKit.c b/Sources/_CJavaScriptKit/_CJavaScriptKit.c index f2f03b82d..0bcc5eaca 100644 --- a/Sources/_CJavaScriptKit/_CJavaScriptKit.c +++ b/Sources/_CJavaScriptKit/_CJavaScriptKit.c @@ -1,17 +1,18 @@ #include "_CJavaScriptKit.h" #include +#include #if __wasm32__ -void _call_host_function_impl(const JavaScriptHostFuncRef host_func_ref, +bool _call_host_function_impl(const JavaScriptHostFuncRef host_func_ref, const RawJSValue *argv, const int argc, const JavaScriptObjectRef callback_func); __attribute__((export_name("swjs_call_host_function"))) -void swjs_call_host_function(const JavaScriptHostFuncRef host_func_ref, +bool swjs_call_host_function(const JavaScriptHostFuncRef host_func_ref, const RawJSValue *argv, const int argc, const JavaScriptObjectRef callback_func) { - _call_host_function_impl(host_func_ref, argv, argc, callback_func); + return _call_host_function_impl(host_func_ref, argv, argc, callback_func); } void _free_host_function_impl(const JavaScriptHostFuncRef host_func_ref); @@ -36,7 +37,7 @@ void swjs_cleanup_host_function_call(void *argv_buffer) { /// this and `SwiftRuntime.version` in `./Runtime/src/index.ts`. __attribute__((export_name("swjs_library_version"))) int swjs_library_version(void) { - return 707; + return 708; } int _library_features(void); diff --git a/Sources/_CJavaScriptKit/include/_CJavaScriptKit.h b/Sources/_CJavaScriptKit/include/_CJavaScriptKit.h index c3b56c14d..fb07d1e09 100644 --- a/Sources/_CJavaScriptKit/include/_CJavaScriptKit.h +++ b/Sources/_CJavaScriptKit/include/_CJavaScriptKit.h @@ -268,10 +268,13 @@ extern bool _instanceof(const JavaScriptObjectRef obj, /// See also comments on JSFunction.swift /// /// @param host_func_id The target Swift side function called by the created thunk function. +/// @param line The line where the function is created. Will be used for diagnostics +/// @param file The file name where the function is created. Will be used for diagnostics /// @returns A reference to the newly-created JavaScript thunk function __attribute__((__import_module__("javascript_kit"), __import_name__("swjs_create_function"))) -extern JavaScriptObjectRef _create_function(const JavaScriptHostFuncRef host_func_id); +extern JavaScriptObjectRef _create_function(const JavaScriptHostFuncRef host_func_id, + unsigned int line, JavaScriptObjectRef file); /// Instantiate a new `TypedArray` object with given elements /// This is used to provide an efficient way to create `TypedArray`.