Macro wrapper for memcpy (#13770)

Initial migration from memcpy to SAFE_MEMCPY Macro and MemBlockBuilder. Fixes #9328 Signed-off-by: grial <[email protected]>
envoyproxy · Mar 16, 2021 · 7852c7b · 7852c7b
1 parent c3c54d4
commit 7852c7b
Show file tree

Hide file tree

Showing 44 changed files with 235 additions and 91 deletions.
diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc
@@ -98,7 +98,7 @@ void OwnedImpl::copyOut(size_t start, uint64_t size, void* data) const {
       continue;
     }
     uint64_t copy_size = std::min(size, data_size - bytes_to_skip);
-    memcpy(dest, slice.data() + bytes_to_skip, copy_size);
+    memcpy(dest, slice.data() + bytes_to_skip, copy_size); // NOLINT(safe-memcpy)
     size -= copy_size;
     dest += copy_size;
     // Now that we've started copying, there are no bytes left to skip over. If there

diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h
@@ -230,7 +230,7 @@ class Slice {
     uint8_t* dest = base_ + reservable_;
     reservable_ += copy_size;
     // NOLINTNEXTLINE(clang-analyzer-core.NullDereference)
-    memcpy(dest, data, copy_size);
+    memcpy(dest, data, copy_size); // NOLINT(safe-memcpy)
     return copy_size;
   }
 
@@ -260,7 +260,7 @@ class Slice {
       copy_size = std::min(size, data_);
       data_ -= copy_size;
     }
-    memcpy(base_ + data_, src + size - copy_size, copy_size);
+    memcpy(base_ + data_, src + size - copy_size, copy_size); // NOLINT(safe-memcpy)
     return copy_size;
   }
 
@@ -788,7 +788,7 @@ class OwnedBufferFragmentImpl final : public BufferFragment, public InlineStorag
   OwnedBufferFragmentImpl(absl::string_view data, const Releasor& releasor)
       : releasor_(releasor), size_(data.size()) {
     ASSERT(releasor != nullptr);
-    memcpy(data_, data.data(), data.size());
+    memcpy(data_, data.data(), data.size()); // NOLINT(safe-memcpy)
   }
 
   const Releasor releasor_;

diff --git a/source/common/chromium_url/BUILD b/source/common/chromium_url/BUILD
@@ -24,5 +24,8 @@ envoy_cc_library(
         "url_parse.h",
         "url_parse_internal.h",
     ],
-    deps = ["//source/common/common:assert_lib"],
+    deps = [
+        "//source/common/common:assert_lib",
+        "//source/common/common:mem_block_builder_lib",
+    ],
 )
diff --git a/source/common/chromium_url/url_canon.h b/source/common/chromium_url/url_canon.h
@@ -13,6 +13,7 @@
 
 #include "common/chromium_url/envoy_shim.h"
 #include "common/chromium_url/url_parse.h"
+#include "common/common/mem_block_builder.h"
 
 namespace chromium_url {
 
@@ -145,11 +146,11 @@ template <typename T, int fixed_capacity = 1024> class RawCanonOutputT : public
   }
 
   void Resize(int sz) override {
-    T* new_buf = new T[sz];
-    memcpy(new_buf, this->buffer_, sizeof(T) * (this->cur_len_ < sz ? this->cur_len_ : sz));
+    Envoy::MemBlockBuilder<T> new_buf(sz);
+    new_buf.appendData(absl::Span<T>(this->buffer, std::min(this->cur_len_, sz)));
     if (this->buffer_ != fixed_buffer_)
       delete[] this->buffer_;
-    this->buffer_ = new_buf;
+    this->buffer_ = new_buf.releasePointer();
     this->buffer_len_ = sz;
   }
 

diff --git a/source/common/common/BUILD b/source/common/common/BUILD
@@ -90,6 +90,11 @@ envoy_cc_library(
     hdrs = ["enum_to_int.h"],
 )
 
+envoy_cc_library(
+    name = "safe_memcpy_lib",
+    hdrs = ["safe_memcpy.h"],
+)
+
 # fmt_lib is automatically a dependency of all envoy_cc_library definitions.
 envoy_basic_cc_library(
     name = "fmt_lib",
@@ -107,6 +112,10 @@ envoy_cc_library(
     srcs = ["hash.cc"],
     hdrs = ["hash.h"],
     external_deps = ["xxhash"],
+    deps = [
+        ":macros",
+        ":safe_memcpy_lib",
+    ],
 )
 
 envoy_cc_library(

diff --git a/source/common/common/hash.h b/source/common/common/hash.h
@@ -2,6 +2,9 @@
 
 #include <string>
 
+#include "common/common/macros.h"
+#include "common/common/safe_memcpy.h"
+
 #include "absl/container/flat_hash_map.h"
 #include "absl/container/flat_hash_set.h"
 #include "absl/strings/ascii.h"
@@ -58,7 +61,7 @@ class MurmurHash {
 private:
   static inline uint64_t unalignedLoad(const char* p) {
     uint64_t result;
-    memcpy(&result, p, sizeof(result));
+    safeMemcpyUnsafeSrc(&result, p);
     return result;
   }
 

diff --git a/source/common/common/mem_block_builder.h b/source/common/common/mem_block_builder.h
@@ -26,7 +26,7 @@ template <class T> class MemBlockBuilder {
 public:
   // Constructs a MemBlockBuilder allowing for 'capacity' instances of T.
   explicit MemBlockBuilder(uint64_t capacity)
-      : data_(std::make_unique<T[]>(capacity)), write_span_(data_.get(), capacity) {}
+      : data_(std::make_unique<T[]>(capacity)), write_span_(data_.get(), capacity){};
   MemBlockBuilder() = default;
 
   /**
@@ -102,6 +102,13 @@ template <class T> class MemBlockBuilder {
     return std::move(data_);
   }
 
+  /**
+   * Returns the underlying storage as a raw pointer, clearing this.
+   *
+   * @return the transferred storage.
+   */
+  T* releasePointer() { return this->release().release(); }
+
   /**
    * @return the populated data as an absl::Span.
    */

diff --git a/source/common/common/safe_memcpy.h b/source/common/common/safe_memcpy.h
@@ -0,0 +1,38 @@
+#pragma once
+
+#include <cstring>
+
+namespace Envoy {
+
+/**
+ * Copies src to dst based on their sizes, which must be the same.
+ * @param dst the destination
+ * @param src the source
+ */
+template <typename T1, typename T2> inline void safeMemcpy(T1* dst, T2* src) {
+  static_assert(sizeof(T1) == sizeof(T2));
+  memcpy(dst, src, sizeof(T1));
+}
+
+/**
+ * Copies src to dst based on the size of dst.
+ * Sizes are not compared, so ensure the src is of size sizeof(*(dst)) before proceeding to
+ * call safeMemcpyUnsafeSrc
+ * @param dst the destination
+ * @param src the source
+ */
+template <typename T1, typename T2> inline void safeMemcpyUnsafeSrc(T1* dst, T2* src) {
+  memcpy(dst, src, sizeof(T1));
+}
+/**
+ * Copies src to dst based on the size of src.
+ * Sizes are not compared, so ensure the dst is of size sizeof(*(src)) before proceeding to
+ * call safeMemcpyUnsafeDst
+ * @param dst the destination
+ * @param src the source
+ */
+template <typename T1, typename T2> inline void safeMemcpyUnsafeDst(T1* dst, T2* src) {
+  memcpy(dst, src, sizeof(T2));
+}
+
+} // namespace Envoy
diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc
@@ -625,7 +625,7 @@ double WelfordStandardDeviation::computeStandardDeviation() const {
 
 InlineString::InlineString(const char* str, size_t size) : size_(size) {
   RELEASE_ASSERT(size <= 0xffffffff, "size must fit in 32 bits");
-  memcpy(data_, str, size);
+  memcpy(data_, str, size); // NOLINT(safe-memcpy)
 }
 
 void ExceptionUtil::throwEnvoyException(const std::string& message) {

diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD
@@ -93,6 +93,7 @@ envoy_cc_library(
         "//source/common/common:enum_to_int",
         "//source/common/common:hash_lib",
         "//source/common/common:macros",
+        "//source/common/common:safe_memcpy_lib",
         "//source/common/common:utility_lib",
         "//source/common/grpc:status_lib",
         "//source/common/http:header_utility_lib",

diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc
@@ -13,6 +13,7 @@
 #include "common/common/enum_to_int.h"
 #include "common/common/fmt.h"
 #include "common/common/macros.h"
+#include "common/common/safe_memcpy.h"
 #include "common/common/utility.h"
 #include "common/http/header_utility.h"
 #include "common/http/headers.h"
@@ -136,7 +137,7 @@ Buffer::InstancePtr Common::serializeToGrpcFrame(const Protobuf::Message& messag
   uint8_t* current = reinterpret_cast<uint8_t*>(reservation.slice().mem_);
   *current++ = 0; // flags
   const uint32_t nsize = htonl(size);
-  std::memcpy(current, reinterpret_cast<const void*>(&nsize), sizeof(uint32_t));
+  safeMemcpyUnsafeDst(current, &nsize);
   current += sizeof(uint32_t);
   Protobuf::io::ArrayOutputStream stream(current, size, -1);
   Protobuf::io::CodedOutputStream codec_stream(&stream);
@@ -288,7 +289,7 @@ void Common::prependGrpcFrameHeader(Buffer::Instance& buffer) {
   std::array<char, 5> header;
   header[0] = 0; // flags
   const uint32_t nsize = htonl(buffer.length());
-  std::memcpy(&header[1], reinterpret_cast<const void*>(&nsize), sizeof(uint32_t));
+  safeMemcpyUnsafeDst(&header[1], &nsize);
   buffer.prepend(absl::string_view(&header[0], 5));
 }
 

diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc
@@ -16,6 +16,7 @@
 #include "common/common/dump_state_utils.h"
 #include "common/common/enum_to_int.h"
 #include "common/common/fmt.h"
+#include "common/common/safe_memcpy.h"
 #include "common/common/scope_tracker.h"
 #include "common/common/utility.h"
 #include "common/http/codes.h"
@@ -789,8 +790,7 @@ Status ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) {
     // was the current time when the ping was sent. This can be useful while debugging
     // to match the ping and ack.
     uint64_t data;
-    static_assert(sizeof(data) == sizeof(frame->ping.opaque_data), "Sizes are equal");
-    memcpy(&data, frame->ping.opaque_data, sizeof(data));
+    safeMemcpy(&data, &(frame->ping.opaque_data));
     ENVOY_CONN_LOG(trace, "recv PING ACK {}", connection_, data);
 
     onKeepaliveResponse();

diff --git a/source/common/network/BUILD b/source/common/network/BUILD
@@ -17,6 +17,7 @@ envoy_cc_library(
         "//include/envoy/network:address_interface",
         "//source/common/api:os_sys_calls_lib",
         "//source/common/common:assert_lib",
+        "//source/common/common:safe_memcpy_lib",
         "//source/common/common:utility_lib",
     ],
 )
@@ -40,6 +41,7 @@ envoy_cc_library(
         ":utility_lib",
         "//include/envoy/network:address_interface",
         "//source/common/common:assert_lib",
+        "//source/common/common:safe_memcpy_lib",
         "//source/common/common:utility_lib",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
     ],

diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc
@@ -9,6 +9,7 @@
 
 #include "common/common/assert.h"
 #include "common/common/fmt.h"
+#include "common/common/safe_memcpy.h"
 #include "common/common/utility.h"
 #include "common/network/socket_interface.h"
 
@@ -175,8 +176,7 @@ std::string Ipv4Instance::sockaddrToString(const sockaddr_in& addr) {
 absl::uint128 Ipv6Instance::Ipv6Helper::address() const {
   absl::uint128 result{0};
   static_assert(sizeof(absl::uint128) == 16, "The size of asbl::uint128 is not 16.");
-  memcpy(static_cast<void*>(&result), static_cast<const void*>(&address_.sin6_addr.s6_addr),
-         sizeof(absl::uint128));
+  safeMemcpyUnsafeSrc(&result, &address_.sin6_addr.s6_addr[0]);
   return result;
 }
 
@@ -281,7 +281,9 @@ PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode,
     }
     pipe_.abstract_namespace_ = true;
     pipe_.address_length_ = pipe_path.size();
-    memcpy(&pipe_.address_.sun_path[0], pipe_path.data(), pipe_path.size());
+    // The following statement is safe since pipe_path size was checked at the beginning of this
+    // function
+    memcpy(&pipe_.address_.sun_path[0], pipe_path.data(), pipe_path.size()); // NOLINT(safe-memcpy)
     pipe_.address_.sun_path[0] = '\0';
     pipe_.address_.sun_path[pipe_path.size()] = '\0';
     friendly_name_ = friendlyNameFromAbstractPath(

diff --git a/source/common/network/address_impl.h b/source/common/network/address_impl.h
@@ -227,6 +227,7 @@ class PipeInstance : public InstanceBase {
   const sockaddr* sockAddr() const override {
     return reinterpret_cast<const sockaddr*>(&pipe_.address_);
   }
+  const sockaddr_un& getSockAddr() const { return pipe_.address_; }
   socklen_t sockAddrLen() const override {
     if (pipe_.abstract_namespace_) {
       return offsetof(struct sockaddr_un, sun_path) + pipe_.address_length_;

diff --git a/source/common/network/cidr_range.cc b/source/common/network/cidr_range.cc
@@ -11,6 +11,7 @@
 
 #include "common/common/assert.h"
 #include "common/common/fmt.h"
+#include "common/common/safe_memcpy.h"
 #include "common/common/utility.h"
 #include "common/network/address_impl.h"
 #include "common/network/utility.h"
@@ -183,7 +184,7 @@ InstanceConstSharedPtr CidrRange::truncateIpAddressAndLength(InstanceConstShared
 
     absl::uint128 ip6_htonl = Utility::Ip6htonl(ip6);
     static_assert(sizeof(absl::uint128) == 16, "The size of asbl::uint128 is not 16.");
-    memcpy(&sa6.sin6_addr.s6_addr, &ip6_htonl, sizeof(absl::uint128));
+    safeMemcpy(&sa6.sin6_addr.s6_addr, &ip6_htonl);
     return std::make_shared<Ipv6Instance>(sa6);
   }
   }

diff --git a/source/extensions/common/proxy_protocol/proxy_protocol_header.cc b/source/extensions/common/proxy_protocol/proxy_protocol_header.cc
@@ -66,39 +66,31 @@ void generateV2Header(const std::string& src_addr, const std::string& dst_addr,
   case Network::Address::IpVersion::v4: {
     addr_length[1] = PROXY_PROTO_V2_ADDR_LEN_INET;
     out.add(addr_length, 2);
-
-    uint8_t addrs[8];
-    const auto net_src_addr =
+    const uint32_t net_src_addr =
         Network::Address::Ipv4Instance(src_addr, src_port).ip()->ipv4()->address();
-    const auto net_dst_addr =
+    const uint32_t net_dst_addr =
         Network::Address::Ipv4Instance(dst_addr, dst_port).ip()->ipv4()->address();
-    memcpy(addrs, &net_src_addr, 4);
-    memcpy(&addrs[4], &net_dst_addr, 4);
-    out.add(addrs, 8);
+    out.add(&net_src_addr, 4);
+    out.add(&net_dst_addr, 4);
     break;
   }
   case Network::Address::IpVersion::v6: {
     addr_length[1] = PROXY_PROTO_V2_ADDR_LEN_INET6;
     out.add(addr_length, 2);
-
-    uint8_t addrs[32];
-    const auto net_src_addr =
+    const absl::uint128 net_src_addr =
         Network::Address::Ipv6Instance(src_addr, src_port).ip()->ipv6()->address();
-    const auto net_dst_addr =
+    const absl::uint128 net_dst_addr =
         Network::Address::Ipv6Instance(dst_addr, dst_port).ip()->ipv6()->address();
-    memcpy(addrs, &net_src_addr, 16);
-    memcpy(&addrs[16], &net_dst_addr, 16);
-    out.add(addrs, 32);
+    out.add(&net_src_addr, 16);
+    out.add(&net_dst_addr, 16);
     break;
   }
   }
 
-  uint8_t ports[4];
-  const auto net_src_port = htons(static_cast<uint16_t>(src_port));
-  const auto net_dst_port = htons(static_cast<uint16_t>(dst_port));
-  memcpy(ports, &net_src_port, 2);
-  memcpy(&ports[2], &net_dst_port, 2);
-  out.add(ports, 4);
+  const uint16_t net_src_port = htons(static_cast<uint16_t>(src_port));
+  const uint16_t net_dst_port = htons(static_cast<uint16_t>(dst_port));
+  out.add(&net_src_port, 2);
+  out.add(&net_dst_port, 2);
 }
 
 void generateV2Header(const Network::Address::Ip& source_address,

diff --git a/source/extensions/common/wasm/BUILD b/source/extensions/common/wasm/BUILD
@@ -83,6 +83,7 @@ envoy_cc_library(
         "//include/envoy/server:lifecycle_notifier_interface",
         "//source/common/buffer:buffer_lib",
         "//source/common/common:enum_to_int",
+        "//source/common/common:safe_memcpy_lib",
         "//source/common/config:remote_data_fetcher_lib",
         "//source/common/http:message_lib",
         "//source/common/http:utility_lib",