Intermediate fix for JS KV handle caching - TypedKvMap does lazy look…

…up (#5717)

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

js/ccf-app/src/kv.ts

@@ -41,43 +41,51 @@ import { KvMap, ccf } from "./global.js";
 import { DataConverter } from "./converters.js";
 
 export class TypedKvMap<K, V> {
+  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<K>,
     private vt: DataConverter<V>,
   ) {}
 
   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<K, V> {
-    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<K, V>) => 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<K, V> {
   }
 
   get size(): number {
-    return this.kv.size;
+    return this._kv().size;
   }
 }
 
@@ -109,8 +117,7 @@ export function typedKv<K, V>(
   kt: DataConverter<K>,
   vt: DataConverter<V>,
 ) {
-  const kvMap = typeof nameOrMap === "string" ? ccf.kv[nameOrMap] : nameOrMap;
-  return new TypedKvMap(kvMap, kt, vt);
+  return new TypedKvMap(nameOrMap, kt, vt);
 }
 
 /**

tests/js-custom-authorization/custom_authorization.py

@@ -899,6 +899,18 @@ def was_cached(response):
         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
 
 

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"
+        }
+      }
     }
   }
 }

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 };

tests/js-interpreter-reuse/src/rollup_entry.ts

@@ -1,2 +1,3 @@
 export * from "./cache.js";
 export * from "./di_sample";
+export * from "./global_handle";