Fix wasm memory bug (#2066)

* Fix wasm memory bug --------- Co-authored-by: kamilsa <[email protected]>
qdrvm · Apr 29, 2024 · 58d3ea6 · 58d3ea6
1 parent 6db7b0b
commit 58d3ea6
Show file tree

Hide file tree

Showing 25 changed files with 212 additions and 165 deletions.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -128,8 +128,8 @@ jobs:
         options:
           - name: "Self-hosted: Linux: gcc-12 ASAN"
             run: ./housekeeping/make_build.sh -DCLEAR_OBJS=ON -DCMAKE_TOOLCHAIN_FILE=cmake/toolchain/gcc-12_cxx20.cmake -DASAN=ON
-          - name: "Self-hosted: Linux: clang-15 TSAN WAVM"
-            run: ./housekeeping/make_build.sh -DCLEAR_OBJS=ON -DCMAKE_TOOLCHAIN_FILE=cmake/toolchain/clang-15_cxx20.cmake -DTSAN=ON -DWASM_COMPILER=WAVM
+          - name: "Self-hosted: Linux: gcc-12 TSAN WAVM"
+            run: ./housekeeping/make_build.sh -DCLEAR_OBJS=ON -DCMAKE_TOOLCHAIN_FILE=cmake/toolchain/gcc-12_cxx20.cmake -DTSAN=ON -DWASM_COMPILER=WAVM
           - name: "Self-hosted: Linux: clang-15 UBSAN"
             run: ./housekeeping/make_build.sh -DCLEAR_OBJS=ON -DCMAKE_TOOLCHAIN_FILE=cmake/toolchain/clang-15_cxx20.cmake -DUBSAN=ON
           - name: "Self-hosted: Linux: clang-15 External Project"

diff --git a/core/runtime/binaryen/binaryen_memory_provider.cpp b/core/runtime/binaryen/binaryen_memory_provider.cpp
@@ -5,6 +5,7 @@
  */
 
 #include "runtime/binaryen/binaryen_memory_provider.hpp"
+#include <memory>
 
 #include "runtime/common/memory_allocator.hpp"
 
@@ -40,7 +41,9 @@ namespace kagome::runtime::binaryen {
     auto rei = external_interface_.lock();
     BOOST_ASSERT(rei != nullptr);
     if (rei) {
-      memory_ = memory_factory_->make(rei->getMemory(), config);
+      std::shared_ptr handle = memory_factory_->make(rei->getMemory(), config);
+      memory_ = std::make_shared<Memory>(
+          handle, std::make_unique<MemoryAllocatorImpl>(handle, config));
       return outcome::success();
     }
     return Error::OUTDATED_EXTERNAL_INTERFACE;

diff --git a/core/runtime/binaryen/binaryen_memory_provider.hpp b/core/runtime/binaryen/binaryen_memory_provider.hpp
@@ -37,7 +37,7 @@ namespace kagome::runtime::binaryen {
    private:
     std::weak_ptr<RuntimeExternalInterface> external_interface_;
     std::shared_ptr<const BinaryenMemoryFactory> memory_factory_;
-    std::shared_ptr<MemoryImpl> memory_;
+    std::shared_ptr<Memory> memory_;
   };
 
 }  // namespace kagome::runtime::binaryen

diff --git a/core/runtime/binaryen/memory_impl.cpp b/core/runtime/binaryen/memory_impl.cpp
@@ -16,7 +16,6 @@ namespace kagome::runtime::binaryen {
   MemoryImpl::MemoryImpl(RuntimeExternalInterface::InternalMemory *memory,
                          const MemoryConfig &config)
       : memory_{memory},
-        allocator_{std::make_unique<MemoryAllocator>(*this, config)},
         logger_{log::createLogger("Binaryen Memory", "binaryen")} {
     // TODO(Harrm): #1714 temporary fix because binaryen doesn't recognize
     // our memory resizes from our allocator
@@ -27,14 +26,6 @@ namespace kagome::runtime::binaryen {
     return memory_->pagesMax();
   }
 
-  WasmPointer MemoryImpl::allocate(WasmSize size) {
-    return allocator_->allocate(size);
-  }
-
-  void MemoryImpl::deallocate(WasmPointer ptr) {
-    return allocator_->deallocate(ptr);
-  }
-
   outcome::result<BytesOut> MemoryImpl::view(WasmPointer ptr,
                                              WasmSize size) const {
     if (not memoryCheck(ptr, size, this->size())) {

diff --git a/core/runtime/binaryen/memory_impl.hpp b/core/runtime/binaryen/memory_impl.hpp
@@ -36,7 +36,7 @@ namespace kagome::runtime::binaryen {
    * @note Memory size of this implementation is at least a page size (4096
    * bytes)
    */
-  class MemoryImpl final : public Memory {
+  class MemoryImpl final : public MemoryHandle {
    public:
     MemoryImpl(RuntimeExternalInterface::InternalMemory *memory,
                const MemoryConfig &config);
@@ -46,9 +46,6 @@ namespace kagome::runtime::binaryen {
     MemoryImpl &operator=(MemoryImpl &&move) = delete;
     ~MemoryImpl() override = default;
 
-    WasmPointer allocate(WasmSize size) override;
-    void deallocate(WasmPointer ptr) override;
-
     void resize(WasmSize new_size) override {
       /**
        * We use this condition to avoid
@@ -69,14 +66,8 @@ namespace kagome::runtime::binaryen {
     outcome::result<BytesOut> view(WasmPointer ptr,
                                    WasmSize size) const override;
 
-    // for testing purposes
-    const MemoryAllocator &getAllocator() const {
-      return *allocator_;
-    }
-
    private:
     RuntimeExternalInterface::InternalMemory *memory_;
-    std::unique_ptr<MemoryAllocator> allocator_;
 
     log::Logger logger_;
   };

diff --git a/core/runtime/common/memory_allocator.cpp b/core/runtime/common/memory_allocator.cpp
@@ -22,24 +22,27 @@ namespace kagome::runtime {
 
   constexpr auto kPoisoned{"the allocator has been poisoned"};
 
-  inline uint64_t read_u64(const Memory &memory, WasmPointer ptr) {
+  static uint64_t read_u64(const MemoryHandle &memory, WasmPointer ptr) {
     return boost::endian::load_little_u64(
         memory.view(ptr, sizeof(uint64_t)).value().data());
   }
 
-  inline void write_u64(const Memory &memory, WasmPointer ptr, uint64_t v) {
+  static void write_u64(const MemoryHandle &memory,
+                        WasmPointer ptr,
+                        uint64_t v) {
     boost::endian::store_little_u64(
         memory.view(ptr, sizeof(uint64_t)).value().data(), v);
   }
 
-  MemoryAllocator::MemoryAllocator(Memory &memory, const MemoryConfig &config)
-      : memory_{memory},
+  MemoryAllocatorImpl::MemoryAllocatorImpl(std::shared_ptr<MemoryHandle> memory,
+                                           const MemoryConfig &config)
+      : memory_{std::move(memory)},
         offset_{roundUpAlign(config.heap_base)},
-        max_memory_pages_num_{memory_.pagesMax().value_or(kMaxPages)} {
+        max_memory_pages_num_{memory_->pagesMax().value_or(kMaxPages)} {
     BOOST_ASSERT(max_memory_pages_num_ > 0);
   }
 
-  WasmPointer MemoryAllocator::allocate(WasmSize size) {
+  WasmPointer MemoryAllocatorImpl::allocate(WasmSize size) {
     if (poisoned_) {
       throw std::runtime_error{kPoisoned};
     }
@@ -53,31 +56,31 @@ namespace kagome::runtime {
     uint32_t head_ptr;
     if (auto &list = free_lists_.at(order)) {
       head_ptr = *list;
-      if (*list + sizeof(Header) + size > memory_.size()) {
+      if (*list + sizeof(Header) + size > memory_->size()) {
         throw std::runtime_error{"Header pointer out of memory bounds"};
       }
       list = readFree(*list);
     } else {
       head_ptr = offset_;
       auto next_offset = uint64_t{offset_} + sizeof(Header) + size;
-      if (next_offset > memory_.size()) {
+      if (next_offset > memory_->size()) {
         auto pages = sizeToPages(next_offset);
         if (pages > max_memory_pages_num_) {
           throw std::runtime_error{
               "Memory resize failed, because maximum number of pages is reached."};
         }
-        pages = std::max(pages, 2 * sizeToPages(memory_.size()));
+        pages = std::max(pages, 2 * sizeToPages(memory_->size()));
         pages = std::min<uint64_t>(pages, max_memory_pages_num_);
-        memory_.resize(pages * kMemoryPageSize);
+        memory_->resize(pages * kMemoryPageSize);
       }
       offset_ = next_offset;
     }
-    write_u64(memory_, head_ptr, kOccupied | order);
+    write_u64(*memory_, head_ptr, kOccupied | order);
     poisoned_ = false;
     return head_ptr + sizeof(Header);
   }
 
-  void MemoryAllocator::deallocate(WasmPointer ptr) {
+  void MemoryAllocatorImpl::deallocate(WasmPointer ptr) {
     if (poisoned_) {
       throw std::runtime_error{kPoisoned};
     }
@@ -90,12 +93,12 @@ namespace kagome::runtime {
     auto &list = free_lists_.at(order);
     auto prev = list.value_or(kNil);
     list = head_ptr;
-    write_u64(memory_, head_ptr, prev);
+    write_u64(*memory_, head_ptr, prev);
     poisoned_ = false;
   }
 
-  uint32_t MemoryAllocator::readOccupied(WasmPointer head_ptr) const {
-    auto head = read_u64(memory_, head_ptr);
+  uint32_t MemoryAllocatorImpl::readOccupied(WasmPointer head_ptr) const {
+    auto head = read_u64(*memory_, head_ptr);
     uint32_t order = head;
     if (order >= kOrders) {
       throw std::runtime_error{"order exceed the total number of orders"};
@@ -106,9 +109,9 @@ namespace kagome::runtime {
     return order;
   }
 
-  std::optional<uint32_t> MemoryAllocator::readFree(
+  std::optional<uint32_t> MemoryAllocatorImpl::readFree(
       WasmPointer head_ptr) const {
-    auto head = read_u64(memory_, head_ptr);
+    auto head = read_u64(*memory_, head_ptr);
     if ((head & kOccupied) != 0) {
       throw std::runtime_error{"free list points to a occupied header"};
     }
@@ -119,12 +122,12 @@ namespace kagome::runtime {
     return prev;
   }
 
-  std::optional<WasmSize> MemoryAllocator::getAllocatedChunkSize(
+  std::optional<WasmSize> MemoryAllocatorImpl::getAllocatedChunkSize(
       WasmPointer ptr) const {
     return kMinAllocate << readOccupied(ptr - sizeof(Header));
   }
 
-  size_t MemoryAllocator::getDeallocatedChunksNum() const {
+  size_t MemoryAllocatorImpl::getDeallocatedChunksNum() const {
     size_t size = 0ull;
     for (auto list : free_lists_) {
       while (list) {

diff --git a/core/runtime/common/memory_allocator.hpp b/core/runtime/common/memory_allocator.hpp
@@ -13,7 +13,7 @@
 #include "runtime/types.hpp"
 
 namespace kagome::runtime {
-  class Memory;
+  class MemoryHandle;
 
   // Alignment for pointers, same with substrate:
   // https://github.com/paritytech/substrate/blob/743981a083f244a090b40ccfb5ce902199b55334/primitives/allocator/src/freeing_bump.rs#L56
@@ -35,16 +35,25 @@ namespace kagome::runtime {
     return math::roundUp<kAlignment>(t);
   }
 
+  class MemoryAllocator {
+   public:
+    virtual ~MemoryAllocator() = default;
+
+    virtual WasmPointer allocate(WasmSize size) = 0;
+    virtual void deallocate(WasmPointer ptr) = 0;
+  };
+
   /**
    * Implementation of allocator for the runtime memory
    * Combination of monotonic and free-list allocator
    */
-  class MemoryAllocator final {
+  class MemoryAllocatorImpl final : public MemoryAllocator {
    public:
-    MemoryAllocator(Memory &memory, const struct MemoryConfig &config);
+    MemoryAllocatorImpl(std::shared_ptr<MemoryHandle> memory,
+                        const struct MemoryConfig &config);
 
-    WasmPointer allocate(WasmSize size);
-    void deallocate(WasmPointer ptr);
+    WasmPointer allocate(WasmSize size) override;
+    void deallocate(WasmPointer ptr) override;
 
     /*
       Following methods are needed mostly for testing purposes.
@@ -68,7 +77,7 @@ namespace kagome::runtime {
     std::optional<uint32_t> readFree(WasmPointer ptr) const;
 
    private:
-    Memory &memory_;
+    std::shared_ptr<MemoryHandle> memory_;
 
     std::array<std::optional<uint32_t>, kOrders> free_lists_;
 

diff --git a/core/runtime/memory.hpp b/core/runtime/memory.hpp
@@ -6,13 +6,11 @@
 
 #pragma once
 
-#include <array>
-
 #include <optional>
 
-#include "common/buffer.hpp"
 #include "common/buffer_view.hpp"
 #include "common/literals.hpp"
+#include "runtime/common/memory_allocator.hpp"
 #include "runtime/ptr_size.hpp"
 #include "runtime/types.hpp"
 
@@ -35,29 +33,19 @@ namespace kagome::runtime {
     return (size + kMemoryPageSize - 1) / kMemoryPageSize;
   }
 
-  /** The underlying memory can be accessed through unaligned pointers which
-   * isn't well-behaved in C++. WebAssembly nonetheless expects it to behave
-   * properly. Avoid emitting unaligned load/store by checking for alignment
-   * explicitly, and performing memcpy if unaligned.
-   *
-   * The allocated memory tries to have the same alignment as the memory being
-   * simulated.
+  class MemoryAllocator;
+
+  /**
+   * An interface for a particular WASM engine memory implementation
    */
-  class Memory {
+  class MemoryHandle {
    public:
-    virtual ~Memory() = default;
+    virtual ~MemoryHandle() = default;
 
-    /**
-     * @brief Return the size of the memory
-     */
     virtual WasmSize size() const = 0;
 
     virtual std::optional<WasmSize> pagesMax() const = 0;
 
-    /**
-     * Resizes memory to the given size
-     * @param new_size
-     */
     virtual void resize(WasmSize new_size) = 0;
 
     virtual outcome::result<BytesOut> view(WasmPointer ptr,
@@ -70,35 +58,76 @@ namespace kagome::runtime {
     outcome::result<BytesOut> view(WasmSpan span) const {
       return view(PtrSize{span});
     }
+  };
+
+  /**
+   * A convenience wrapper around a memory handle and a memory allocator.
+   *
+   * Mind that the underlying memory can be accessed through unaligned pointers
+   * which isn't well-behaved in C++. WebAssembly nonetheless expects it to
+   * behave properly. Avoid emitting unaligned load/store by checking for
+   * alignment explicitly, and performing memcpy if unaligned.
+   *
+   * The allocated memory tries to have the same alignment as the memory being
+   * simulated.
+   */
+  class Memory final {
+   public:
+    Memory(std::shared_ptr<MemoryHandle> handle,
+           std::unique_ptr<MemoryAllocator> allocator)
+        : handle_{std::move(handle)}, allocator_{std::move(allocator)} {
+      BOOST_ASSERT(handle_);
+      BOOST_ASSERT(allocator_);
+    }
+
+    outcome::result<BytesOut> view(WasmPointer ptr, WasmSize size) const {
+      return handle_->view(ptr, size);
+    }
+
+    outcome::result<BytesOut> view(PtrSize ptr_size) const {
+      return handle_->view(ptr_size);
+    }
+
+    outcome::result<BytesOut> view(WasmSpan span) const {
+      return handle_->view(span);
+    }
 
     /**
      * Allocates memory of given size and returns address in the memory
      * @param size allocated memory size
      * @return address to allocated memory. If there is no available slot for
      * such allocation, then -1 is returned
      */
-    virtual WasmPointer allocate(WasmSize size) = 0;
+    WasmPointer allocate(WasmSize size) {
+      return allocator_->allocate(size);
+    }
 
     /**
      * Deallocates memory in provided region
      * @param ptr address of deallocated memory
      * @return size of deallocated memory or none if given address does not
      * point to any allocated pieces of memory
      */
-    virtual void deallocate(WasmPointer ptr) = 0;
+    void deallocate(WasmPointer ptr) {
+      return allocator_->deallocate(ptr);
+    }
 
     common::BufferView loadN(WasmPointer ptr, WasmSize size) const {
-      return view(ptr, size).value();
+      return handle_->view(ptr, size).value();
     }
 
     void storeBuffer(WasmPointer ptr, common::BufferView v) {
-      memcpy(view(ptr, v.size()).value().data(), v.data(), v.size());
+      memcpy(handle_->view(ptr, v.size()).value().data(), v.data(), v.size());
     }
 
     WasmSpan storeBuffer(common::BufferView v) {
       auto ptr = allocate(v.size());
       storeBuffer(ptr, v);
       return PtrSize{ptr, static_cast<WasmSize>(v.size())}.combine();
     }
+
+   private:
+    std::shared_ptr<MemoryHandle> handle_;
+    std::unique_ptr<MemoryAllocator> allocator_;
   };
 }  // namespace kagome::runtime