From cd0af68f43e44b3f3ce8c7a8fe7384ce58de6a6a Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Tue, 20 Aug 2019 11:14:38 -0700 Subject: [PATCH] Fix the alignement in optval of setsockopt when compiled with libc++. (#7958) Description: libc++ std::string may inline the data which results the memory is not aligned to `void*`. Use vector instead to store the optval. Detected by UBSAN with libc++ config. Preparation for #4251 Risk Level: Low Testing: unittest locally Docs Changes: N/A Release Notes: N/A Fixes #7968 Signed-off-by: Lizan Zhou Signed-off-by: Piotr Sikora --- source/common/network/socket_option_impl.cc | 10 +++++----- source/common/network/socket_option_impl.h | 12 +++++++++--- .../network/socket_option_factory_test.cc | 17 +++++++++-------- test/common/network/socket_option_impl_test.cc | 3 ++- 4 files changed, 25 insertions(+), 17 deletions(-) diff --git a/source/common/network/socket_option_impl.cc b/source/common/network/socket_option_impl.cc index 2065309ed7a5..a4313490fdaf 100644 --- 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(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 index 4ff917bb70ec..94f11b081b52 100644 --- 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(value_.data()) % alignof(void*) == 0); + } // Socket::Option bool setOption(Socket& socket, @@ -111,16 +114,19 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable but not std::string because std::string might inline + // the buffer so its data() is not aligned in to alignof(void*). + const std::vector value_; }; } // namespace Network diff --git a/test/common/network/socket_option_factory_test.cc b/test/common/network/socket_option_factory_test.cc index d9934195d338..b4b5f0831b26 100644 --- a/test/common/network/socket_option_factory_test.cc +++ b/test/common/network/socket_option_factory_test.cc @@ -154,20 +154,21 @@ TEST_F(SocketOptionFactoryTest, TestBuildLiteralOptions) { EXPECT_TRUE(option_details.has_value()); EXPECT_EQ(SOL_SOCKET, option_details->name_->first); EXPECT_EQ(SO_LINGER, option_details->name_->second); - EXPECT_EQ(sizeof(struct linger), option_details->value_.size()); - const struct linger* linger_ptr = - reinterpret_cast(option_details->value_.data()); - EXPECT_EQ(1, linger_ptr->l_onoff); - EXPECT_EQ(3456, linger_ptr->l_linger); + struct linger expected_linger; + expected_linger.l_onoff = 1; + expected_linger.l_linger = 3456; + absl::string_view linger_bstr{reinterpret_cast(&expected_linger), + sizeof(struct linger)}; + EXPECT_EQ(linger_bstr, option_details->value_); option_details = socket_options->at(1)->getOptionDetails( socket_mock_, envoy::api::v2::core::SocketOption::STATE_PREBIND); EXPECT_TRUE(option_details.has_value()); EXPECT_EQ(SOL_SOCKET, option_details->name_->first); EXPECT_EQ(SO_KEEPALIVE, option_details->name_->second); - EXPECT_EQ(sizeof(int), option_details->value_.size()); - const int* flag_ptr = reinterpret_cast(option_details->value_.data()); - EXPECT_EQ(1, *flag_ptr); + int value = 1; + absl::string_view value_bstr{reinterpret_cast(&value), sizeof(int)}; + EXPECT_EQ(value_bstr, option_details->value_); } } // namespace diff --git a/test/common/network/socket_option_impl_test.cc b/test/common/network/socket_option_impl_test.cc index aa066522bde6..8d39beba1a59 100644 --- 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_); }