review: stop copying source

Signed-off-by: Takeshi Yoneda <[email protected]>

Author: mathetake

src/common/wasm_util.cc

@@ -13,16 +13,21 @@
 // limitations under the License.
 
 #include "src/common/wasm_util.h"
-#include <cstdio>
-#include <string.h>
+#include <cstring>
 
 namespace proxy_wasm {
 namespace common {
 
-bool WasmUtil::getCustomSection(const char *begin, const char *end, std::string_view name,
+bool WasmUtil::checkWasmHeader(std::string_view bytecode) {
+  static const uint8_t wasm_magic_number[4] = {0x00, 0x61, 0x73, 0x6d};
+  return bytecode.size() < 8 || !::memcmp(bytecode.data(), wasm_magic_number, 4);
+}
+
+bool WasmUtil::getCustomSection(std::string_view bytecode, std::string_view name,
                                 std::string_view &ret) {
   // Skip the Wasm header.
-  const char *pos = begin + 8;
+  const char *pos = bytecode.data() + 8;
+  const char *end = bytecode.data() + bytecode.size();
   while (pos < end) {
     if (pos + 1 > end) {
       return false;
@@ -53,10 +58,10 @@ bool WasmUtil::getCustomSection(const char *begin, const char *end, std::string_
   return true;
 };
 
-bool WasmUtil::getFunctionNameIndex(const char *begin, const char *end,
+bool WasmUtil::getFunctionNameIndex(std::string_view bytecode,
                                     std::unordered_map<uint32_t, std::string> &ret) {
   std::string_view name_section = {};
-  if (!WasmUtil::getCustomSection(begin, end, "name", name_section)) {
+  if (!WasmUtil::getCustomSection(bytecode, "name", name_section)) {
     return false;
   };
   if (!name_section.empty()) {
@@ -101,9 +106,10 @@ bool WasmUtil::getFunctionNameIndex(const char *begin, const char *end,
   return true;
 }
 
-bool WasmUtil::getStrippedSource(const char *begin, const char *end, std::vector<char> &ret) {
+bool WasmUtil::getStrippedSource(std::string_view bytecode, std::string &ret) {
   // Skip the Wasm header.
-  const char *pos = begin + 8;
+  const char *pos = bytecode.data() + 8;
+  const char *end = bytecode.data() + bytecode.size();
   while (pos < end) {
     const auto section_start = pos;
     if (pos + 1 > end) {
@@ -125,7 +131,7 @@ bool WasmUtil::getStrippedSource(const char *begin, const char *end, std::vector
         // If this is the first "precompiled_" section, then save everything
         // before it, otherwise skip it.
         if (ret.empty()) {
-          const char *start = begin;
+          const char *start = bytecode.data();
           ret.insert(ret.end(), start, section_start);
         }
       }

src/common/wasm_util.h

@@ -23,6 +23,7 @@ namespace common {
 // Utilitiy functions which directly operate on Wasm bytecodes.
 class WasmUtil {
 public:
+  static bool checkWasmHeader(std::string_view bytecode);
   /**
    * getCustomSection extract the view of the custom section for a given name.
    * @param begin is the beggining of bytecode.
@@ -31,7 +32,7 @@ class WasmUtil {
    * @param ret is the reference to store the resulting view to the custom section.
    * @return indicates whether parsing succeeded or not.
    */
-  static bool getCustomSection(const char *begin, const char *end, std::string_view name,
+  static bool getCustomSection(std::string_view bytecode, std::string_view name,
                                std::string_view &ret);
 
   /**
@@ -43,7 +44,7 @@ class WasmUtil {
    * @param ret the reference to store map from function indexes to function names.
    * @return indicates whether parsing succeeded or not.
    */
-  static bool getFunctionNameIndex(const char *begin, const char *end,
+  static bool getFunctionNameIndex(std::string_view bytecode,
                                    std::unordered_map<uint32_t, std::string> &ret);
 
   /**
@@ -53,15 +54,9 @@ class WasmUtil {
    * @param ret the reference to the stripped bytecode.
    * @return indicates whether parsing succeeded or not.
    */
-  static bool getStrippedSource(const char *begin, const char *end, std::vector<char> &ret);
+  static bool getStrippedSource(std::string_view bytecode, std::string &ret);
 
-  /**
-   * parseVarint parses the bytecode following LEB128.
-   * @param begin is the postion where parsing starts.
-   * @param end is the end of bytecode.
-   * @param ret is the reference to store the resulting value.
-   * @return indicates whether parsing succeeded or not.
-   */
+private:
   static bool parseVarint(const char *&begin, const char *end, uint32_t &ret);
 };
 

src/v8/v8.cc

@@ -113,7 +113,6 @@ class V8 : public WasmVm {
   void getModuleFunctionImpl(std::string_view function_name,
                              std::function<R(ContextBase *, Args...)> *function);
 
-  wasm::vec<byte_t> source_ = wasm::vec<byte_t>::invalid();
   wasm::own<wasm::Store> store_;
   wasm::own<wasm::Module> module_;
   wasm::own<wasm::Shared<wasm::Module>> shared_module_;
@@ -255,22 +254,15 @@ bool V8::load(const std::string &code, bool allow_precompiled) {
   store_ = wasm::Store::make(engine());
 
   // Wasm file header is 8 bytes (magic number + version).
-  static const uint8_t magic_number[4] = {0x00, 0x61, 0x73, 0x6d};
-  if (code.size() < 8 || ::memcmp(code.data(), magic_number, 4) != 0) {
+  if (!common::WasmUtil::checkWasmHeader(code)) {
     return false;
   }
 
-  source_ = wasm::vec<byte_t>::make_uninitialized(code.size());
-  ::memcpy(source_.get(), code.data(), code.size());
-
-  const char *source_begin = source_.get();
-  const char *source_end = source_begin + source_.size();
   if (allow_precompiled) {
     const auto section_name = getPrecompiledSectionName();
     if (!section_name.empty()) {
-      string_view precompiled = {};
-      if (!common::WasmUtil::getCustomSection(source_begin, source_end, section_name,
-                                              precompiled)) {
+      std::string_view precompiled = {};
+      if (!common::WasmUtil::getCustomSection(code, section_name, precompiled)) {
         fail(FailState::UnableToInitializeCode, "Failed to parse corrupted Wasm module");
         return false;
       }
@@ -289,19 +281,28 @@ bool V8::load(const std::string &code, bool allow_precompiled) {
   }
 
   if (!module_) {
-    const std::vector<char> stripped =
-        common::WasmUtil::getStrippedSource(source_begin, source_end);
-    module_ = wasm::Module::make(
-        store_.get(),
-        stripped.empty() ? source_ : wasm::vec<byte_t>::make(stripped.size(), stripped.data()));
+    std::string stripped;
+    if (!common::WasmUtil::getStrippedSource(code, stripped)) {
+      fail(FailState::UnableToInitializeCode, "Failed to parse corrupted Wasm module");
+      return false;
+    };
+    wasm::vec<byte_t> code_vec = wasm::vec<byte_t>::invalid();
+    if (stripped.empty()) {
+      // Use the original bytecode.
+      code_vec = wasm::vec<byte_t>::make(code.size(), (char *)(code.data()));
+    } else {
+      // Othewise use the stripped bytecode.
+      code_vec = wasm::vec<byte_t>::make(stripped.size(), stripped.data());
+    }
+    module_ = wasm::Module::make(store_.get(), code_vec);
   }
 
   if (module_) {
     shared_module_ = module_->share();
     assert((shared_module_ != nullptr));
   }
 
-  if (!common::WasmUtil::getFunctionNameIndex(source_begin, source_end, function_names_index_)) {
+  if (!common::WasmUtil::getFunctionNameIndex(code, function_names_index_)) {
     fail(FailState::UnableToInitializeCode, "Failed to parse corrupted Wasm module");
   };
   return module_ != nullptr;

src/wasmtime/wasmtime.cc

@@ -99,7 +99,6 @@ class Wasmtime : public WasmVm {
   void getModuleFunctionImpl(std::string_view function_name,
                              std::function<R(ContextBase *, Args...)> *function);
 
-  WasmByteVec source_;
   WasmStorePtr store_;
   WasmModulePtr module_;
   WasmSharedModulePtr shared_module_;
@@ -115,29 +114,25 @@ class Wasmtime : public WasmVm {
 bool Wasmtime::load(const std::string &code, bool allow_precompiled) {
   store_ = wasm_store_new(engine());
 
-  // Wasm file header is 8 bytes (magic number + version).
-  static const uint8_t magic_number[4] = {0x00, 0x61, 0x73, 0x6d};
-  if (code.size() < 8 || ::memcmp(code.data(), magic_number, 4) != 0) {
+  if (!common::WasmUtil::checkWasmHeader(code)) {
     return false;
   }
 
-  wasm_byte_vec_new_uninitialized(source_.get(), code.size());
-  ::memcpy(source_.get()->data, code.data(), code.size());
-
-  std::vector<char> stripped_vec;
-  if (!common::WasmUtil::getStrippedSource(
-          source_.get()->data, source_.get()->data + source_.get()->size, stripped_vec)) {
+  std::vector<uint8_t> stripped_vec;
+  if (!common::WasmUtil::getStrippedSource(code, stripped_vec)) {
     fail(FailState::UnableToInitializeCode, "Failed to parse corrupted Wasm module");
+    return false;
   };
 
   if (stripped_vec.empty()) {
-    // Use the original source.
-    module_ = wasm_module_new(store_.get(), source_.get());
+    // Use the original bytecode.
+    WasmByteVec source_vec;
+    wasm_byte_vec_new(source_vec.get(), code.size(), code.data());
+    module_ = wasm_module_new(store_.get(), source_vec.get());
   } else {
     // Othewise pass the stripped source code.
     WasmByteVec stripped;
-    wasm_byte_vec_new_uninitialized(stripped.get(), stripped_vec.size());
-    ::memcpy(stripped.get()->data, stripped_vec.data(), stripped_vec.size());
+    wasm_byte_vec_new(stripped.get(), code.size(), code.data());
     module_ = wasm_module_new(store_.get(), stripped.get());
   }
 

test/common/wasm_util_test.cc

@@ -26,7 +26,7 @@ namespace proxy_wasm {
 namespace common {
 
 TEST(TestWasmCommonUtil, getCustomSection) {
-  std::vector<char> custom_section = {
+  std::string custom_section = {
       0x00, 0x61, 0x73, 0x6d,       // Wasm magic
       0x01, 0x00, 0x00, 0x00,       // Wasm version
       0x00,                         // custom section id
@@ -37,48 +37,47 @@ TEST(TestWasmCommonUtil, getCustomSection) {
   std::string_view section = {};
 
   // OK.
-  EXPECT_TRUE(WasmUtil::getCustomSection(
-      custom_section.data(), custom_section.data() + custom_section.size(), "hey!", section));
+  EXPECT_TRUE(WasmUtil::getCustomSection(custom_section, "hey!", section));
   EXPECT_EQ(std::string(section), "hello");
   section = {};
 
   // Non exist.
-  EXPECT_TRUE(WasmUtil::getCustomSection(
-      custom_section.data(), custom_section.data() + custom_section.size(), "non-exist", section));
+  EXPECT_TRUE(WasmUtil::getCustomSection(custom_section, "non-exist", section));
   EXPECT_EQ(section, "");
 
   // Fail due to the corrupted bytecode.
   // TODO(@mathetake): here we haven't covered all the parsing failure branches. Add more cases.
-  EXPECT_FALSE(WasmUtil::getCustomSection(
-      custom_section.data(), custom_section.data() + custom_section.size() - 3, "hey", section));
-  EXPECT_FALSE(WasmUtil::getCustomSection(
-      custom_section.data() + 1, custom_section.data() + custom_section.size(), "hey", section));
+  std::string corrupted = {custom_section.data(),
+                           custom_section.data() + custom_section.size() - 3};
+  EXPECT_FALSE(WasmUtil::getCustomSection(corrupted, "hey", section));
+  corrupted = {custom_section.data() + 1, custom_section.data() + custom_section.size()};
+  EXPECT_FALSE(WasmUtil::getCustomSection(corrupted, "hey", section));
 }
 
 TEST(TestWasmCommonUtil, getFunctionNameIndex) {
   const auto source = readTestWasmFile("abi_export.wasm");
   std::unordered_map<uint32_t, std::string> actual;
   // OK.
-  EXPECT_TRUE(WasmUtil::getFunctionNameIndex(source.data(), source.data() + source.size(), actual));
+  EXPECT_TRUE(WasmUtil::getFunctionNameIndex(source, actual));
   EXPECT_FALSE(actual.empty());
   EXPECT_EQ(actual.find(0)->second, "proxy_abi_version_0_2_0");
 
   // Fail due to the corrupted bytecode.
   // TODO(@mathetake): here we haven't covered all the parsing failure branches. Add more cases.
   actual = {};
   std::string_view name_section = {};
-  EXPECT_TRUE(WasmUtil::getCustomSection(source.data(), source.data() + source.size(), "name",
-                                         name_section));
+  EXPECT_TRUE(WasmUtil::getCustomSection(source, "name", name_section));
   // Passing module with malformed custom section.
-  EXPECT_FALSE(WasmUtil::getFunctionNameIndex(source.data(), name_section.data() + 1, actual));
+  std::string corrupted = {source.data(), name_section.data() + 1};
+  EXPECT_FALSE(WasmUtil::getFunctionNameIndex(corrupted, actual));
   EXPECT_TRUE(actual.empty());
 }
 
 TEST(TestWasmCommonUtil, getStrippedSource) {
   // Unmodified case.
   auto source = readTestWasmFile("abi_export.wasm");
-  std::vector<char> actual;
-  EXPECT_TRUE(WasmUtil::getStrippedSource(source.data(), source.data() + source.size(), actual));
+  std::vector<uint8_t> actual;
+  EXPECT_TRUE(WasmUtil::getStrippedSource(source, actual));
   // No `precompiled_` is found in the custom sections.
   EXPECT_TRUE(actual.empty());
 
@@ -97,42 +96,16 @@ TEST(TestWasmCommonUtil, getStrippedSource) {
 
   source.append(custom_section.data(), custom_section.size());
   std::string_view section = {};
-  EXPECT_TRUE(WasmUtil::getCustomSection(source.data(), source.data() + source.size(),
-                                         "precompiled_test", section));
+  EXPECT_TRUE(WasmUtil::getCustomSection(source, "precompiled_test", section));
   EXPECT_FALSE(section.empty());
 
   // Chcek if the custom section is stripped.
   actual = {};
-  EXPECT_TRUE(WasmUtil::getStrippedSource(source.data(), source.data() + source.size(), actual));
+  EXPECT_TRUE(WasmUtil::getStrippedSource(source, actual));
   // No `precompiled_` is found in the custom sections.
   EXPECT_FALSE(actual.empty());
   EXPECT_EQ(actual.size(), source.size() - custom_section.size());
 }
 
-TEST(WasmUtil, parseVarint) {
-  std::vector<char> source;
-  const char *begin = source.data();
-  uint32_t ret = 0;
-  EXPECT_FALSE(WasmUtil::parseVarint(begin, source.data(), ret));
-
-  source = {0x04};
-  begin = source.data();
-  EXPECT_TRUE(WasmUtil::parseVarint(begin, begin + source.size(), ret));
-  EXPECT_EQ(0x04, ret);
-  ret = 0;
-
-  source = {static_cast<char>(0x80), 0x7f};
-  begin = source.data();
-  EXPECT_TRUE(WasmUtil::parseVarint(begin, begin + source.size(), ret));
-  EXPECT_EQ(16256, ret);
-  ret = 0;
-
-  source = {static_cast<char>(0x89), static_cast<char>(0x80), static_cast<char>(0x80),
-            static_cast<char>(0x80), 0x01};
-  begin = source.data();
-  EXPECT_TRUE(WasmUtil::parseVarint(begin, begin + source.size(), ret));
-  EXPECT_EQ(268435465, ret);
-}
-
 } // namespace common
 } // namespace proxy_wasm