From eed5eaa44d24c9619d301a8bd65041b2418020d4 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Mon, 19 Aug 2019 07:02:37 +0000 Subject: [PATCH 1/2] Fix the alignement in optval of setsockopt when compiled with libc++. Description: libc++ std::string may inline the data which results the memory is not aligned to max_align_t. 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 Signed-off-by: Lizan Zhou --- source/common/network/socket_option_impl.cc | 12 ++++++------ 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, 26 insertions(+), 18 deletions(-) diff --git a/source/common/network/socket_option_impl.cc b/source/common/network/socket_option_impl.cc index 65b364810f21..dbf8aa153301 100644 --- a/source/common/network/socket_option_impl.cc +++ b/source/common/network/socket_option_impl.cc @@ -19,7 +19,7 @@ bool SocketOptionImpl::setOption(Socket& socket, } 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: {}", optname_.name(), strerror(result.errno_)); @@ -39,22 +39,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, const 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.level(), optname.option(), - value.data(), value.size()); + return os_syscalls.setsockopt(socket.ioHandle().fd(), optname.level(), optname.option(), value, + size); } } // namespace Network diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h index 59931506323c..4fa210738bef 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 { @@ -103,7 +104,9 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable(value_.data()) % alignof(std::max_align_t) == 0); + } // Socket::Option bool setOption(Socket& socket, @@ -123,17 +126,20 @@ 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(std::max_align_t). + 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 6ec4767ab372..ca5e25cc363f 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_.level()); EXPECT_EQ(SO_LINGER, option_details->name_.option()); - 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_.level()); EXPECT_EQ(SO_KEEPALIVE, option_details->name_.option()); - 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 8d9e55ae2d19..d979d8a01098 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_); } From 910c9d745b4d7ab3ba9a2a879872c43d4bad2de6 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Mon, 19 Aug 2019 20:11:04 +0000 Subject: [PATCH 2/2] alignment should be void* Signed-off-by: Lizan Zhou --- source/common/network/socket_option_impl.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h index 4fa210738bef..1a13a67010cb 100644 --- a/source/common/network/socket_option_impl.h +++ b/source/common/network/socket_option_impl.h @@ -105,7 +105,7 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable(value_.data()) % alignof(std::max_align_t) == 0); + ASSERT(reinterpret_cast(value_.data()) % alignof(void*) == 0); } // Socket::Option @@ -138,7 +138,7 @@ 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(std::max_align_t). + // the buffer so its data() is not aligned in to alignof(void*). const std::vector value_; };