wasm: fix root_context_id that was incorrectly cached across threads.…

… (#15016) Fixes proxy-wasm/proxy-wasm-cpp-host#125. Signed-off-by: Piotr Sikora <[email protected]>
rexengineering · Feb 17, 2021 · d7f9cbc · d7f9cbc
1 parent 2863a99
commit d7f9cbc
Show file tree

Hide file tree

Showing 10 changed files with 74 additions and 40 deletions.
diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl
@@ -921,8 +921,8 @@ REPOSITORY_LOCATIONS_SPEC = dict(
         project_name = "WebAssembly for Proxies (C++ host implementation)",
         project_desc = "WebAssembly for Proxies (C++ host implementation)",
         project_url = "https://github.com/proxy-wasm/proxy-wasm-cpp-host",
-        version = "5a53cf4b231599e1d2a1f2f4598fdfbb727ff948",
-        sha256 = "600dbc651a2837e6f1db964eb7e1078e5e338049a34c9ab47415dfa7f3de5478",
+        version = "c51fbca35e9e7968fc5319258ed7a38b1bc1ec7a",
+        sha256 = "533944a9084c2f75c36bda627152b9f31047ff3554b6361a88a542f16dee9483",
         strip_prefix = "proxy-wasm-cpp-host-{version}",
         urls = ["https://github.com/proxy-wasm/proxy-wasm-cpp-host/archive/{version}.tar.gz"],
         use_category = ["dataplane_ext"],
@@ -937,7 +937,7 @@ REPOSITORY_LOCATIONS_SPEC = dict(
             "envoy.wasm.runtime.wavm",
             "envoy.wasm.runtime.wasmtime",
         ],
-        release_date = "2021-01-12",
+        release_date = "2021-02-11",
         cpe = "N/A",
     ),
     proxy_wasm_rust_sdk = dict(

diff --git a/source/extensions/common/wasm/context.h b/source/extensions/common/wasm/context.h
@@ -48,6 +48,7 @@ using GrpcService = envoy::config::core::v3::GrpcService;
 
 class Wasm;
 
+using PluginBaseSharedPtr = std::shared_ptr<PluginBase>;
 using PluginHandleBaseSharedPtr = std::shared_ptr<PluginHandleBase>;
 using WasmHandleBaseSharedPtr = std::shared_ptr<WasmHandleBase>;
 

diff --git a/source/extensions/common/wasm/wasm.cc b/source/extensions/common/wasm/wasm.cc
@@ -282,10 +282,12 @@ getCloneFactory(WasmExtension* wasm_extension, Event::Dispatcher& dispatcher,
 
 static proxy_wasm::PluginHandleFactory getPluginFactory(WasmExtension* wasm_extension) {
   auto wasm_plugin_factory = wasm_extension->pluginFactory();
-  return [wasm_plugin_factory](WasmHandleBaseSharedPtr base_wasm,
-                               absl::string_view plugin_key) -> std::shared_ptr<PluginHandleBase> {
-    return wasm_plugin_factory(std::static_pointer_cast<WasmHandle>(base_wasm), plugin_key);
-  };
+  return
+      [wasm_plugin_factory](WasmHandleBaseSharedPtr base_wasm,
+                            PluginBaseSharedPtr base_plugin) -> std::shared_ptr<PluginHandleBase> {
+        return wasm_plugin_factory(std::static_pointer_cast<WasmHandle>(base_wasm),
+                                   std::static_pointer_cast<Plugin>(base_plugin));
+      };
 }
 
 WasmEvent toWasmEvent(const std::shared_ptr<WasmHandleBase>& wasm) {

diff --git a/source/extensions/common/wasm/wasm.h b/source/extensions/common/wasm/wasm.h
@@ -142,15 +142,19 @@ using WasmHandleSharedPtr = std::shared_ptr<WasmHandle>;
 
 class PluginHandle : public PluginHandleBase, public ThreadLocal::ThreadLocalObject {
 public:
-  explicit PluginHandle(const WasmHandleSharedPtr& wasm_handle, absl::string_view plugin_key)
-      : PluginHandleBase(std::static_pointer_cast<WasmHandleBase>(wasm_handle), plugin_key),
-        wasm_handle_(wasm_handle) {}
+  explicit PluginHandle(const WasmHandleSharedPtr& wasm_handle, const PluginSharedPtr& plugin)
+      : PluginHandleBase(std::static_pointer_cast<WasmHandleBase>(wasm_handle),
+                         std::static_pointer_cast<PluginBase>(plugin)),
+        wasm_handle_(wasm_handle),
+        root_context_id_(wasm_handle->wasm()->getRootContext(plugin, false)->id()) {}
 
   WasmSharedPtr& wasm() { return wasm_handle_->wasm(); }
   WasmHandleSharedPtr& wasmHandleForTest() { return wasm_handle_; }
+  uint32_t rootContextId() { return root_context_id_; }
 
 private:
   WasmHandleSharedPtr wasm_handle_;
+  const uint32_t root_context_id_;
 };
 
 using PluginHandleSharedPtr = std::shared_ptr<PluginHandle>;

diff --git a/source/extensions/common/wasm/wasm_extension.cc b/source/extensions/common/wasm/wasm_extension.cc
@@ -40,10 +40,10 @@ RegisterWasmExtension::RegisterWasmExtension(WasmExtension* extension) {
 }
 
 PluginHandleExtensionFactory EnvoyWasm::pluginFactory() {
-  return [](const WasmHandleSharedPtr& base_wasm,
-            absl::string_view plugin_key) -> PluginHandleBaseSharedPtr {
+  return [](const WasmHandleSharedPtr& wasm_handle,
+            const PluginSharedPtr& plugin) -> PluginHandleBaseSharedPtr {
     return std::static_pointer_cast<PluginHandleBase>(
-        std::make_shared<PluginHandle>(base_wasm, plugin_key));
+        std::make_shared<PluginHandle>(wasm_handle, plugin));
   };
 }
 

diff --git a/source/extensions/common/wasm/wasm_extension.h b/source/extensions/common/wasm/wasm_extension.h
@@ -32,7 +32,7 @@ using WasmHandleSharedPtr = std::shared_ptr<WasmHandle>;
 using CreateContextFn =
     std::function<ContextBase*(Wasm* wasm, const std::shared_ptr<Plugin>& plugin)>;
 using PluginHandleExtensionFactory = std::function<PluginHandleBaseSharedPtr(
-    const WasmHandleSharedPtr& base_wasm, absl::string_view plugin_key)>;
+    const WasmHandleSharedPtr& wasm_handle, const PluginSharedPtr& plugin)>;
 using WasmHandleExtensionFactory = std::function<WasmHandleBaseSharedPtr(
     const VmConfig& vm_config, const CapabilityRestrictionConfig& capability_restriction_config,
     const Stats::ScopeSharedPtr& scope, Upstream::ClusterManager& cluster_manager,

diff --git a/source/extensions/filters/http/wasm/wasm_filter.h b/source/extensions/filters/http/wasm/wasm_filter.h
@@ -31,17 +31,19 @@ class FilterConfig : Logger::Loggable<Logger::Id::wasm> {
     if (handle.has_value()) {
       wasm = handle->wasm().get();
     }
-    if (plugin_->fail_open_ && (!wasm || wasm->isFailed())) {
-      return nullptr;
+    if (!wasm || wasm->isFailed()) {
+      if (plugin_->fail_open_) {
+        // Fail open skips adding this filter to callbacks.
+        return nullptr;
+      } else {
+        // Fail closed is handled by an empty Context.
+        return std::make_shared<Context>(nullptr, 0, plugin_);
+      }
     }
-    if (wasm && !root_context_id_) {
-      root_context_id_ = wasm->getRootContext(plugin_, false)->id();
-    }
-    return std::make_shared<Context>(wasm, root_context_id_, plugin_);
+    return std::make_shared<Context>(wasm, handle->rootContextId(), plugin_);
   }
 
 private:
-  uint32_t root_context_id_{0};
   PluginSharedPtr plugin_;
   ThreadLocal::TypedSlotPtr<PluginHandle> tls_slot_;
   Config::DataSource::RemoteAsyncDataProviderPtr remote_data_provider_;

diff --git a/source/extensions/filters/network/wasm/wasm_filter.h b/source/extensions/filters/network/wasm/wasm_filter.h
@@ -31,19 +31,21 @@ class FilterConfig : Logger::Loggable<Logger::Id::wasm> {
     if (handle.has_value()) {
       wasm = handle->wasm().get();
     }
-    if (plugin_->fail_open_ && (!wasm || wasm->isFailed())) {
-      return nullptr;
+    if (!wasm || wasm->isFailed()) {
+      if (plugin_->fail_open_) {
+        // Fail open skips adding this filter to callbacks.
+        return nullptr;
+      } else {
+        // Fail closed is handled by an empty Context.
+        return std::make_shared<Context>(nullptr, 0, plugin_);
+      }
     }
-    if (wasm && !root_context_id_) {
-      root_context_id_ = wasm->getRootContext(plugin_, false)->id();
-    }
-    return std::make_shared<Context>(wasm, root_context_id_, plugin_);
+    return std::make_shared<Context>(wasm, handle->rootContextId(), plugin_);
   }
 
   Wasm* wasmForTest() { return tls_slot_->get()->wasm().get(); }
 
 private:
-  uint32_t root_context_id_{0};
   PluginSharedPtr plugin_;
   ThreadLocal::TypedSlotPtr<PluginHandle> tls_slot_;
   Config::DataSource::RemoteAsyncDataProviderPtr remote_data_provider_;

diff --git a/test/extensions/common/wasm/wasm_test.cc b/test/extensions/common/wasm/wasm_test.cc
@@ -711,10 +711,10 @@ TEST_P(WasmCommonTest, VmCache) {
             });
         return std::make_shared<WasmHandle>(wasm);
       },
-      [](const WasmHandleBaseSharedPtr& base_wasm,
-         absl::string_view plugin_key) -> PluginHandleBaseSharedPtr {
-        return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(base_wasm),
-                                              plugin_key);
+      [](const WasmHandleBaseSharedPtr& wasm_handle,
+         const PluginBaseSharedPtr& plugin) -> PluginHandleBaseSharedPtr {
+        return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(wasm_handle),
+                                              std::static_pointer_cast<Plugin>(plugin));
       });
   wasm_handle.reset();
   wasm_handle2.reset();
@@ -818,10 +818,10 @@ TEST_P(WasmCommonTest, RemoteCode) {
             });
         return std::make_shared<WasmHandle>(wasm);
       },
-      [](const WasmHandleBaseSharedPtr& base_wasm,
-         absl::string_view plugin_key) -> PluginHandleBaseSharedPtr {
-        return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(base_wasm),
-                                              plugin_key);
+      [](const WasmHandleBaseSharedPtr& wasm_handle,
+         const PluginBaseSharedPtr& plugin) -> PluginHandleBaseSharedPtr {
+        return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(wasm_handle),
+                                              std::static_pointer_cast<Plugin>(plugin));
       });
   wasm_handle.reset();
 
@@ -936,10 +936,10 @@ TEST_P(WasmCommonTest, RemoteCodeMultipleRetry) {
             });
         return std::make_shared<WasmHandle>(wasm);
       },
-      [](const WasmHandleBaseSharedPtr& base_wasm,
-         absl::string_view plugin_key) -> PluginHandleBaseSharedPtr {
-        return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(base_wasm),
-                                              plugin_key);
+      [](const WasmHandleBaseSharedPtr& wasm_handle,
+         const PluginBaseSharedPtr& plugin) -> PluginHandleBaseSharedPtr {
+        return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(wasm_handle),
+                                              std::static_pointer_cast<Plugin>(plugin));
       });
   wasm_handle.reset();
 

diff --git a/test/extensions/filters/network/wasm/config_test.cc b/test/extensions/filters/network/wasm/config_test.cc
@@ -154,6 +154,29 @@ TEST_P(WasmNetworkFilterConfigTest, YamlLoadInlineBadCodeFailOpenNackConfig) {
                             "Unable to create Wasm network filter test");
 }
 
+TEST_P(WasmNetworkFilterConfigTest, FilterConfigFailClosed) {
+  if (GetParam() == "null") {
+    return;
+  }
+  const std::string yaml = TestEnvironment::substitute(absl::StrCat(R"EOF(
+  config:
+    vm_config:
+      runtime: "envoy.wasm.runtime.)EOF",
+                                                                    GetParam(), R"EOF("
+      code:
+        local:
+          filename: "{{ test_rundir }}/test/extensions/filters/network/wasm/test_data/test_cpp.wasm"
+  )EOF"));
+
+  envoy::extensions::filters::network::wasm::v3::Wasm proto_config;
+  TestUtility::loadFromYaml(yaml, proto_config);
+  NetworkFilters::Wasm::FilterConfig filter_config(proto_config, context_);
+  filter_config.wasmForTest()->fail(proxy_wasm::FailState::RuntimeError, "");
+  auto context = filter_config.createFilter();
+  EXPECT_EQ(context->wasm(), nullptr);
+  EXPECT_TRUE(context->isFailed());
+}
+
 TEST_P(WasmNetworkFilterConfigTest, FilterConfigFailOpen) {
   if (GetParam() == "null") {
     return;