Skip to content

Commit

Permalink
Fix the alignement in optval of setsockopt when compiled with libc++. (
Browse files Browse the repository at this point in the history
…envoyproxy#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 envoyproxy#4251

Risk Level: Low
Testing: unittest locally
Docs Changes: N/A
Release Notes: N/A
Fixes envoyproxy#7968

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
  • Loading branch information
lizan authored and PiotrSikora committed Aug 20, 2019
1 parent 6910569 commit d1188b6
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 9 deletions.
10 changes: 5 additions & 5 deletions source/common/network/socket_option_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down
12 changes: 9 additions & 3 deletions source/common/network/socket_option_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion test/common/network/socket_option_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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_);
}
Expand Down

0 comments on commit d1188b6

Please sign in to comment.