diff --git a/CHANGELOG.md b/CHANGELOG.md index b8db9730c8..efb2aca2b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## [5.0.0-dev4] + +[5.0.0-dev4]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.0-dev4 + +- Fix for JS execution behaviour when reusing interpreters. Storing KV handles on the global state may lead to unsafe accesses. Work around that by lazily requesting handles in the TypedKvMap for TypeScript apps. + ## [5.0.0-dev3] [5.0.0-dev3]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.0-dev3 diff --git a/js/ccf-app/src/kv.ts b/js/ccf-app/src/kv.ts index 662ebf2672..d358e19e42 100644 --- a/js/ccf-app/src/kv.ts +++ b/js/ccf-app/src/kv.ts @@ -41,43 +41,51 @@ import { KvMap, ccf } from "./global.js"; import { DataConverter } from "./converters.js"; export class TypedKvMap { + private _kv() { + const kvMap = + typeof this.nameOrMap === "string" + ? ccf.kv[this.nameOrMap] + : this.nameOrMap; + return kvMap; + } + constructor( - private kv: KvMap, + private nameOrMap: string | KvMap, private kt: DataConverter, private vt: DataConverter, ) {} has(key: K): boolean { - return this.kv.has(this.kt.encode(key)); + return this._kv().has(this.kt.encode(key)); } get(key: K): V | undefined { - const v = this.kv.get(this.kt.encode(key)); + const v = this._kv().get(this.kt.encode(key)); return v === undefined ? undefined : this.vt.decode(v); } getVersionOfPreviousWrite(key: K): number | undefined { - return this.kv.getVersionOfPreviousWrite(this.kt.encode(key)); + return this._kv().getVersionOfPreviousWrite(this.kt.encode(key)); } set(key: K, value: V): TypedKvMap { - this.kv.set(this.kt.encode(key), this.vt.encode(value)); + this._kv().set(this.kt.encode(key), this.vt.encode(value)); return this; } delete(key: K): void { - this.kv.delete(this.kt.encode(key)); + this._kv().delete(this.kt.encode(key)); } clear(): void { - this.kv.clear(); + this._kv().clear(); } forEach(callback: (value: V, key: K, table: TypedKvMap) => void): void { let kt = this.kt; let vt = this.vt; let typedMap = this; - this.kv.forEach(function ( + this._kv().forEach(function ( raw_v: ArrayBuffer, raw_k: ArrayBuffer, table: KvMap, @@ -87,7 +95,7 @@ export class TypedKvMap { } get size(): number { - return this.kv.size; + return this._kv().size; } } @@ -109,8 +117,7 @@ export function typedKv( kt: DataConverter, vt: DataConverter, ) { - const kvMap = typeof nameOrMap === "string" ? ccf.kv[nameOrMap] : nameOrMap; - return new TypedKvMap(kvMap, kt, vt); + return new TypedKvMap(nameOrMap, kt, vt); } /** diff --git a/tests/js-custom-authorization/custom_authorization.py b/tests/js-custom-authorization/custom_authorization.py index cfd88be27a..6b024871ba 100644 --- a/tests/js-custom-authorization/custom_authorization.py +++ b/tests/js-custom-authorization/custom_authorization.py @@ -899,6 +899,18 @@ def test_reused_interpreter_behaviour(network, args): expect_much_smaller(repeat2, baseline) expect_much_smaller(repeat3, baseline) + LOG.info("Testing caching of KV handles") + r = c.post("/app/increment") + assert r.status_code == http.HTTPStatus.OK, r + r = c.post("/app/increment") + assert r.status_code == http.HTTPStatus.OK, r + r = c.post("/app/increment") + assert r.status_code == http.HTTPStatus.OK, r + r = c.post("/app/increment") + assert r.status_code == http.HTTPStatus.OK, r + r = c.post("/app/increment") + assert r.status_code == http.HTTPStatus.OK, r + return network diff --git a/tests/js-interpreter-reuse/app.json b/tests/js-interpreter-reuse/app.json index 7a4ec98389..876965b0e0 100644 --- a/tests/js-interpreter-reuse/app.json +++ b/tests/js-interpreter-reuse/app.json @@ -61,6 +61,19 @@ "key": "arbitrary_string_goes_here" } } + }, + "/increment": { + "post": { + "js_module": "global_handle.js", + "js_function": "increment", + "forwarding_required": "never", + "authn_policies": ["no_auth"], + "mode": "readwrite", + "openapi": {}, + "interpreter_reuse": { + "key": "increment" + } + } } } } diff --git a/tests/js-interpreter-reuse/src/global_handle.ts b/tests/js-interpreter-reuse/src/global_handle.ts new file mode 100644 index 0000000000..07c591afd0 --- /dev/null +++ b/tests/js-interpreter-reuse/src/global_handle.ts @@ -0,0 +1,25 @@ +import * as ccfapp from "@microsoft/ccf-app"; + +function increment() { + if (!("kvHandle" in globalThis)) { + globalThis.kvHandle = ccfapp.typedKv( + "public:cached_handle_table", + ccfapp.string, + ccfapp.uint32, + ); + } + + const k = "single_key"; + if (!globalThis.kvHandle.has(k)) { + globalThis.kvHandle.set(k, 0); + } else { + const v = globalThis.kvHandle.get(k); + globalThis.kvHandle.set(k, v + 1); + } + + const v = globalThis.kvHandle.get(k); + + return { body: { value: v } }; +} + +export { increment }; diff --git a/tests/js-interpreter-reuse/src/rollup_entry.ts b/tests/js-interpreter-reuse/src/rollup_entry.ts index aa1303bb04..6fd965ce85 100644 --- a/tests/js-interpreter-reuse/src/rollup_entry.ts +++ b/tests/js-interpreter-reuse/src/rollup_entry.ts @@ -1,2 +1,3 @@ export * from "./cache.js"; export * from "./di_sample"; +export * from "./global_handle";