Don't store temporary pointers in JSValues (#5740)

.daily_canary

@@ -1,4 +1,4 @@
    -^-     ___           ___
   (- -)   (= =)   |     Y   &  +--?
  (  V  ) /  .  \  |    +---=---'
-/--x-m- /--n-n---xXx--/--yY------>>>----<<<>>]]{{}}
+/--x-m- /--n-n---xXx--/--yY------>>>----<<<>>]]{{}}--

.threading_canary

@@ -1 +1 @@
-........
+..........

include/ccf/kv/read_only_store.h

@@ -16,6 +16,7 @@ namespace kv
 
     virtual ccf::TxID get_txid() = 0;
     virtual kv::ReadOnlyTx create_read_only_tx() = 0;
+    virtual std::unique_ptr<kv::ReadOnlyTx> create_read_only_tx_ptr() = 0;
     virtual kv::TxDiff create_tx_diff() = 0;
   };
 

js/ccf-app/src/kv.ts

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

samples/apps/logging/js/src/logging.js

@@ -41,7 +41,7 @@ function delete_record(map, id) {
   return { body: true };
 }
 
-export function get_private(request, scope) {
+export function get_private(request) {
   const parsedQuery = parse_request_query(request);
   const id = get_id_from_query(parsedQuery);
   return get_record(private_records(ccf.kv, parsedQuery.scope), id);

src/apps/js_generic/js_generic_base.cpp

@@ -220,16 +220,21 @@ namespace ccfapp
       }
       request.set("params", std::move(params));
 
-      const auto& request_body = endpoint_ctx.rpc_ctx->get_request_body();
       auto body_ = ctx.new_obj_class(js::body_class_id);
-      JS_SetOpaque(body_, (void*)&request_body);
+      ctx.globals.current_request_body =
+        &endpoint_ctx.rpc_ctx->get_request_body();
       request.set("body", std::move(body_));
 
       request.set("caller", create_caller_obj(endpoint_ctx, ctx));
 
       return request;
     }
 
+    void invalidate_request_obj_body(js::Context& ctx)
+    {
+      ctx.globals.current_request_body = nullptr;
+    }
+
     void execute_request(
       const ccf::js::JSDynamicEndpoint* endpoint,
       ccf::endpoints::EndpointContext& endpoint_ctx)
@@ -246,14 +251,10 @@ namespace ccfapp
           [this, endpoint](
             ccf::endpoints::EndpointContext& endpoint_ctx,
             ccf::historical::StatePtr state) {
-            auto tx = state->store->create_read_only_tx();
-            auto tx_id = state->transaction_id;
-            auto receipt = state->receipt;
-            assert(receipt);
-            js::ReadOnlyTxContext historical_txctx{&tx};
             auto add_historical_globals = [&](js::Context& ctx) {
-              js::populate_global_ccf_historical_state(
-                &historical_txctx, tx_id, receipt, ctx);
+              auto ccf = ctx.get_global_property("ccf");
+              auto val = ccf::js::create_historical_state_object(ctx, state);
+              ccf.set("historicalState", std::move(val));
             };
             do_execute_request(endpoint, endpoint_ctx, add_historical_globals);
           },
@@ -318,10 +319,8 @@ namespace ccfapp
       JS_SetModuleLoaderFunc(
         ctx.runtime(), nullptr, js::js_app_module_loader, &endpoint_ctx.tx);
 
-      js::TxContext txctx{&endpoint_ctx.tx};
-
       js::register_request_body_class(ctx);
-      js::populate_global_ccf_kv(&txctx, ctx);
+      js::populate_global_ccf_kv(endpoint_ctx.tx, ctx);
 
       js::populate_global_ccf_rpc(endpoint_ctx.rpc_ctx.get(), ctx);
       js::populate_global_ccf_host(
@@ -361,7 +360,12 @@ namespace ccfapp
         &endpoint_ctx.tx,
         ccf::js::RuntimeLimitsPolicy::NONE);
 
-      auto& rt = ctx.runtime();
+      // Clear globals (which potentially reference locals like txctx), from
+      // this potentially reused interpreter
+      invalidate_request_obj_body(ctx);
+      js::invalidate_globals(ctx);
+
+      const auto& rt = ctx.runtime();
 
       if (JS_IsException(val))
       {

src/js/historical.cpp

@@ -4,11 +4,11 @@
 
 namespace ccf::js
 {
-  static JSValue ccf_receipt_to_js(js::Context* jsctx, TxReceiptImplPtr receipt)
+  static JSValue ccf_receipt_to_js(js::Context& jsctx, TxReceiptImplPtr receipt)
   {
     ccf::ReceiptPtr receipt_out_p = ccf::describe_receipt_v2(*receipt);
     auto& receipt_out = *receipt_out_p;
-    auto js_receipt = jsctx->new_obj();
+    auto js_receipt = jsctx.new_obj();
     JS_CHECK_EXC(js_receipt);
 
     std::string sig_b64;
@@ -18,18 +18,18 @@ namespace ccf::js
     }
     catch (const std::exception& e)
     {
-      return jsctx->new_internal_error(
+      return jsctx.new_internal_error(
         "Failed to convert signature to base64: %s", e.what());
     }
-    auto sig_string = jsctx->new_string(sig_b64.c_str());
+    auto sig_string = jsctx.new_string(sig_b64.c_str());
     JS_CHECK_EXC(sig_string);
     JS_CHECK_SET(js_receipt.set("signature", std::move(sig_string)));
 
-    auto js_cert = jsctx->new_string(receipt_out.cert.str().c_str());
+    auto js_cert = jsctx.new_string(receipt_out.cert.str().c_str());
     JS_CHECK_EXC(js_cert);
     JS_CHECK_SET(js_receipt.set("cert", std::move(js_cert)));
 
-    auto js_node_id = jsctx->new_string(receipt_out.node_id.value().c_str());
+    auto js_node_id = jsctx.new_string(receipt_out.node_id.value().c_str());
     JS_CHECK_EXC(js_node_id);
     JS_CHECK_SET(js_receipt.set("node_id", std::move(js_node_id)));
     bool is_signature_transaction = receipt_out.is_signature_transaction();
@@ -40,18 +40,18 @@ namespace ccf::js
     {
       auto p_receipt =
         std::dynamic_pointer_cast<ccf::ProofReceipt>(receipt_out_p);
-      auto leaf_components = jsctx->new_obj();
+      auto leaf_components = jsctx.new_obj();
       JS_CHECK_EXC(leaf_components);
 
       const auto wsd_hex =
         ds::to_hex(p_receipt->leaf_components.write_set_digest.h);
 
-      auto js_wsd = jsctx->new_string(wsd_hex.c_str());
+      auto js_wsd = jsctx.new_string(wsd_hex.c_str());
       JS_CHECK_EXC(js_wsd);
       JS_CHECK_SET(leaf_components.set("write_set_digest", std::move(js_wsd)));
 
       auto js_commit_evidence =
-        jsctx->new_string(p_receipt->leaf_components.commit_evidence.c_str());
+        jsctx.new_string(p_receipt->leaf_components.commit_evidence.c_str());
       JS_CHECK_EXC(js_commit_evidence);
       JS_CHECK_SET(
         leaf_components.set("commit_evidence", std::move(js_commit_evidence)));
@@ -61,26 +61,26 @@ namespace ccf::js
         const auto cd_hex =
           ds::to_hex(p_receipt->leaf_components.claims_digest.value().h);
 
-        auto js_cd = jsctx->new_string(cd_hex.c_str());
+        auto js_cd = jsctx.new_string(cd_hex.c_str());
         JS_CHECK_EXC(js_cd);
         JS_CHECK_SET(leaf_components.set("claims_digest", std::move(js_cd)));
       }
       JS_CHECK_SET(
         js_receipt.set("leaf_components", std::move(leaf_components)));
 
-      auto proof = jsctx->new_array();
+      auto proof = jsctx.new_array();
       JS_CHECK_EXC(proof);
 
       uint32_t i = 0;
       for (auto& element : p_receipt->proof)
       {
-        auto js_element = jsctx->new_obj();
+        auto js_element = jsctx.new_obj();
         JS_CHECK_EXC(js_element);
 
         auto is_left = element.direction == ccf::ProofReceipt::ProofStep::Left;
         const auto hash_hex = ds::to_hex(element.hash.h);
 
-        auto js_hash = jsctx->new_string(hash_hex.c_str());
+        auto js_hash = jsctx.new_string(hash_hex.c_str());
         JS_CHECK_EXC(js_hash);
         JS_CHECK_SET(
           js_element.set(is_left ? "left" : "right", std::move(js_hash)));
@@ -94,21 +94,14 @@ namespace ccf::js
         std::dynamic_pointer_cast<ccf::SignatureReceipt>(receipt_out_p);
       const auto signed_root = sig_receipt->signed_root;
       const auto root_hex = ds::to_hex(signed_root.h);
-      auto js_root_hex = jsctx->new_string(root_hex.c_str());
+      auto js_root_hex = jsctx.new_string(root_hex.c_str());
       JS_CHECK_EXC(js_root_hex);
       JS_CHECK_SET(js_receipt.set("root_hex", std::move(js_root_hex)));
     }
 
     return js_receipt.take();
   }
 
-  static void js_historical_state_finalizer(JSRuntime* rt, JSValue val)
-  {
-    auto* state_ctx =
-      (HistoricalStateContext*)JS_GetOpaque(val, historical_state_class_id);
-    delete state_ctx;
-  }
-
   static JSValue js_historical_get_state_range(
     JSContext* ctx, JSValueConst this_val, int argc, JSValueConst* argv)
   {
@@ -171,38 +164,16 @@ namespace ccf::js
       return ccf::js::constants::Null;
     }
 
-    ccf::js::Context* jsctx = (ccf::js::Context*)JS_GetContextOpaque(ctx);
+    js::Context& jsctx = *(js::Context*)JS_GetContextOpaque(ctx);
 
-    auto states_array = jsctx->new_array();
+    auto states_array = jsctx.new_array();
     JS_CHECK_EXC(states_array);
     size_t i = 0;
     for (auto& state : states)
     {
-      auto js_state = jsctx->new_obj_class(historical_state_class_id);
-      JS_CHECK_EXC(js_state);
-
-      // Note: The state_ctx object is deleted by js_historical_state_finalizer
-      // which is registered as the finalizer for historical_state_class_id.
-      auto state_ctx = new HistoricalStateContext{
-        state, state->store->create_read_only_tx(), ReadOnlyTxContext{nullptr}};
-      state_ctx->tx_ctx.tx = &state_ctx->tx;
-      JS_SetOpaque(js_state, state_ctx);
-
-      auto transaction_id =
-        jsctx->new_string(state->transaction_id.to_str().c_str());
-      JS_CHECK_EXC(transaction_id);
-      JS_CHECK_SET(js_state.set("transactionId", std::move(transaction_id)));
-
-      auto js_receipt = (*jsctx)(ccf_receipt_to_js(jsctx, state->receipt));
-      JS_CHECK_EXC(js_receipt);
-      JS_CHECK_SET(js_state.set("receipt", std::move(js_receipt)));
-
-      auto kv = jsctx->new_obj_class(kv_read_only_class_id);
-      JS_CHECK_EXC(kv);
-      JS_SetOpaque(kv, &state_ctx->tx_ctx);
-      JS_CHECK_SET(js_state.set("kv", std::move(kv)));
-
-      states_array.set_at_index(i++, std::move(js_state));
+      auto js_state =
+        jsctx(ccf::js::create_historical_state_object(jsctx, state));
+      JS_CHECK_SET(states_array.set_at_index(i++, std::move(js_state)));
     }
 
     return states_array.take();