Merge pull request #91 from PiotrSikora/asan-1.1

network: fix misaligned memory access in setsockopt().
yxue · Aug 20, 2019 · 2a03059 · 2a03059
2 parents 6910569 + d1188b6
commit 2a03059
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 9 deletions.
diff --git a/source/common/network/socket_option_impl.cc b/source/common/network/socket_option_impl.cc
@@ -14,7 +14,7 @@ bool SocketOptionImpl::setOption(Socket& socket,
                                  envoy::api::v2::core::SocketOption::SocketState state) const {
   if (in_state_ == state) {
     const Api::SysCallIntResult result =
-        SocketOptionImpl::setSocketOption(socket, optname_, value_);
+        SocketOptionImpl::setSocketOption(socket, optname_, value_.data(), value_.size());
     if (result.rc_ != 0) {
       ENVOY_LOG(warn, "Setting option on socket failed: {}", strerror(result.errno_));
       return false;
@@ -32,22 +32,22 @@ SocketOptionImpl::getOptionDetails(const Socket&,
 
   Socket::Option::Details info;
   info.name_ = optname_;
-  info.value_ = value_;
-  return absl::optional<Option::Details>(std::move(info));
+  info.value_ = {value_.begin(), value_.end()};
+  return absl::make_optional(std::move(info));
 }
 
 bool SocketOptionImpl::isSupported() const { return optname_.has_value(); }
 
 Api::SysCallIntResult SocketOptionImpl::setSocketOption(Socket& socket,
                                                         Network::SocketOptionName optname,
-                                                        const absl::string_view value) {
+                                                        const void* value, size_t size) {
 
   if (!optname.has_value()) {
     return {-1, ENOTSUP};
   }
   auto& os_syscalls = Api::OsSysCallsSingleton::get();
   return os_syscalls.setsockopt(socket.ioHandle().fd(), optname.value().first,
-                                optname.value().second, value.data(), value.size());
+                                optname.value().second, value, size);
 }
 
 } // namespace Network

diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h
@@ -7,6 +7,7 @@
 #include "envoy/api/os_sys_calls.h"
 #include "envoy/network/listen_socket.h"
 
+#include "common/common/assert.h"
 #include "common/common/logger.h"
 
 namespace Envoy {
@@ -91,7 +92,9 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable<Logger::Id::con
 
   SocketOptionImpl(envoy::api::v2::core::SocketOption::SocketState in_state,
                    Network::SocketOptionName optname, absl::string_view value)
-      : in_state_(in_state), optname_(optname), value_(value) {}
+      : in_state_(in_state), optname_(optname), value_(value.begin(), value.end()) {
+    ASSERT(reinterpret_cast<uintptr_t>(value_.data()) % alignof(void*) == 0);
+  }
 
   // Socket::Option
   bool setOption(Socket& socket,
@@ -111,16 +114,19 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable<Logger::Id::con
    * @param socket the socket on which to apply the option.
    * @param optname the option name.
    * @param value the option value.
+   * @param size the option value size.
    * @return a Api::SysCallIntResult with rc_ = 0 for success and rc = -1 for failure. If the call
    * is successful, errno_ shouldn't be used.
    */
   static Api::SysCallIntResult setSocketOption(Socket& socket, Network::SocketOptionName optname,
-                                               absl::string_view value);
+                                               const void* value, size_t size);
 
 private:
   const envoy::api::v2::core::SocketOption::SocketState in_state_;
   const Network::SocketOptionName optname_;
-  const std::string value_;
+  // This has to be a std::vector<uint8_t> but not std::string because std::string might inline
+  // the buffer so its data() is not aligned in to alignof(void*).
+  const std::vector<uint8_t> value_;
 };
 
 } // namespace Network

diff --git a/test/common/network/socket_option_impl_test.cc b/test/common/network/socket_option_impl_test.cc
@@ -8,7 +8,8 @@ class SocketOptionImplTest : public SocketOptionTest {};
 
 TEST_F(SocketOptionImplTest, BadFd) {
   absl::string_view zero("\0\0\0\0", 4);
-  Api::SysCallIntResult result = SocketOptionImpl::setSocketOption(socket_, {}, zero);
+  Api::SysCallIntResult result =
+      SocketOptionImpl::setSocketOption(socket_, {}, zero.data(), zero.size());
   EXPECT_EQ(-1, result.rc_);
   EXPECT_EQ(ENOTSUP, result.errno_);
 }