You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2022/02/15 00:27:50 UTC
[kudu] branch branch-1.16.x updated: KUDU-3352 fix handling sockaddr_in in Sockaddr
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch branch-1.16.x
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/branch-1.16.x by this push:
new 2a8d35c KUDU-3352 fix handling sockaddr_in in Sockaddr
2a8d35c is described below
commit 2a8d35cf9b77356659689256d46a37bf7063c570
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Thu Feb 10 19:56:35 2022 -0800
KUDU-3352 fix handling sockaddr_in in Sockaddr
Since the Sockaddr class has been updated to handle various address
families, an issue with uninitialized memory in the storage_.in.sin_zero
started affecting the behavior of the BytewiseCompare() and HashCode()
methods. As the result, ReactorThread::FindOrStartConnection() could
create a new connection to the specified remote even if a connection
with the required credentials policy had already been established.
This patch addresses the issue by not counting the padding 'sin_zero'
field of the sockaddr_in structure in the length of Sockaddr::storage_
which is passed to Sockaddr::{BytewiseCompare,HashCode}().
I also added a new test scenario to reproduce the issue, verifying that
the scenario fails if not applying the corresponding part of this patch
to src/kudu/util/net/sockaddr.cc:
[ RUN ] SockaddrHashTest.ZeroPadding
src/kudu/util/net/socket-test.cc:69: Failure
Expected equality of these values:
s_in.HashCode()
Which is: 2868988679
s_un.HashCode()
Which is: 1786951233
[ FAILED ] SockaddrHashTest.ZeroPadding (0 ms)
Credit goes to Wenzhe Zhou for detecting and troubleshooting the issue.
This is a follow-up to 79079b6b64295b06c5a0f05ed200a55d89bccc47.
Change-Id: I96fee11a4191a4f4240c460db22742880eebe7b5
Reviewed-on: http://gerrit.cloudera.org:8080/18223
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Wenzhe Zhou <wz...@cloudera.com>
Reviewed-by: Attila Bukor <ab...@apache.org>
(cherry picked from commit 136a8c21549c9d81d96d8ef3cfa4d200b9325f32)
Reviewed-on: http://gerrit.cloudera.org:8080/18230
Tested-by: Kudu Jenkins
---
src/kudu/util/net/sockaddr.cc | 35 ++++++++++++++++++++++++------
src/kudu/util/net/sockaddr.h | 4 ++++
src/kudu/util/net/socket-test.cc | 46 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 79 insertions(+), 6 deletions(-)
diff --git a/src/kudu/util/net/sockaddr.cc b/src/kudu/util/net/sockaddr.cc
index d34b8ec..06b7482 100644
--- a/src/kudu/util/net/sockaddr.cc
+++ b/src/kudu/util/net/sockaddr.cc
@@ -70,7 +70,9 @@ Sockaddr Sockaddr::Wildcard() {
}
Sockaddr& Sockaddr::operator=(const Sockaddr& other) noexcept {
- if (&other == this) return *this;
+ if (&other == this) {
+ return *this;
+ }
set_length(other.len_);
memcpy(&storage_, &other.storage_, len_);
return *this;
@@ -81,8 +83,14 @@ Sockaddr::Sockaddr(const struct sockaddr& addr, socklen_t len) {
memcpy(&storage_, &addr, len);
}
+// When storing sockaddr_in, do not count the padding sin_zero field in the
+// length of Sockaddr::storage_ since the padding might contain irrelevant
+// non-zero bytes that should not be passed along with the rest of the valid
+// and properly initialized sockaddr_in's fields to BytewiseCompare() and
+// HashCode().
Sockaddr::Sockaddr(const struct sockaddr_in& addr) :
- Sockaddr(reinterpret_cast<const struct sockaddr&>(addr), sizeof(addr)) {
+ Sockaddr(reinterpret_cast<const struct sockaddr&>(addr),
+ offsetof(struct sockaddr_in, sin_zero)) {
DCHECK_EQ(AF_INET, addr.sin_family);
}
@@ -97,9 +105,20 @@ Status Sockaddr::ParseFromNumericHostPort(const HostPort& hp) {
if (inet_pton(AF_INET, hp.host().c_str(), &addr) != 1) {
return Status::InvalidArgument("Invalid IP address", hp.host());
}
- set_length(sizeof(struct sockaddr_in));
+ // Do not count the padding sin_zero field in the length of Sockaddr::storage_
+ // since the padding might contain irrelevant non-zero bytes that should not
+ // be passed along with the rest of the valid and properly initialized
+ // sockaddr_in's fields to BytewiseCompare() and HashCode().
+ constexpr auto len = offsetof(struct sockaddr_in, sin_zero);
+ set_length(len);
+ static_assert(len > offsetof(struct sockaddr_in, sin_addr));
storage_.in.sin_family = AF_INET;
+ static_assert(len > offsetof(struct sockaddr_in, sin_family));
storage_.in.sin_addr = addr;
+#ifndef __linux__
+ static_assert(len > offsetof(struct sockaddr_in, sin_len));
+ storage_.in.sin_len = sizeof(struct sockaddr_in);
+#endif
set_port(hp.port());
return Status::OK();
}
@@ -165,8 +184,12 @@ Sockaddr::UnixAddressType Sockaddr::unix_address_type() const {
}
Sockaddr& Sockaddr::operator=(const struct sockaddr_in &addr) {
- set_length(sizeof(addr));
- memcpy(&storage_, &addr, sizeof(addr));
+ // Do not count the padding sin_zero field since it contains irrelevant bytes
+ // that should not be passed along with the rest of the valid and properly
+ // initialized fields into storage_.
+ constexpr auto len = offsetof(struct sockaddr_in, sin_zero);
+ set_length(len);
+ memcpy(&storage_, &addr, len);
DCHECK_EQ(family(), AF_INET);
return *this;
}
@@ -191,7 +214,7 @@ void Sockaddr::set_length(socklen_t len) {
}
uint32_t Sockaddr::HashCode() const {
- return HashStringThoroughly(reinterpret_cast<const char*>(&storage_), addrlen());
+ return HashStringThoroughly(reinterpret_cast<const char*>(&storage_), len_);
}
void Sockaddr::set_port(int port) {
diff --git a/src/kudu/util/net/sockaddr.h b/src/kudu/util/net/sockaddr.h
index 6fb9e59..8b3d86d 100644
--- a/src/kudu/util/net/sockaddr.h
+++ b/src/kudu/util/net/sockaddr.h
@@ -135,8 +135,12 @@ class Sockaddr {
const struct sockaddr* addr() const {
return reinterpret_cast<const sockaddr*>(&storage_);
}
+
socklen_t addrlen() const {
DCHECK(is_initialized());
+ if (family() == AF_INET) {
+ return sizeof(struct sockaddr_in);
+ }
return len_;
}
diff --git a/src/kudu/util/net/socket-test.cc b/src/kudu/util/net/socket-test.cc
index fb22db0..9ff1dad 100644
--- a/src/kudu/util/net/socket-test.cc
+++ b/src/kudu/util/net/socket-test.cc
@@ -44,6 +44,52 @@ namespace kudu {
constexpr size_t kEchoChunkSize = 32 * 1024 * 1024;
+// A test scenario to make sure Sockaddr::HashCode() works as expected for
+// addresses which are logically the same. In essence, the implementation of
+// the Sockaddr class should zero out the zero padding field
+// sockaddr_in::sin_zero to avoid issues related to not-initialized and former
+// contents of the memory backing the zero padding field.
+TEST(SockaddrHashTest, ZeroPadding) {
+ constexpr const char* const kIpAddr = "127.0.0.1";
+ constexpr const char* const kPath = "/tmp/some/long/enough/path/to.sock";
+ constexpr uint16_t kPort = 5678;
+
+ Sockaddr s_in;
+ ASSERT_OK(s_in.ParseString(kIpAddr, kPort));
+
+ Sockaddr s_un;
+ ASSERT_OK(s_un.ParseUnixDomainPath(kPath));
+
+ // Make 's_un' to be logically the same object as 's_in', but reusing the
+ // Sockaddr::storage_ field from its prior incarnation.
+ ASSERT_OK(s_un.ParseString(kIpAddr, kPort));
+
+ // The hash should be the same since 's_in' and 's_un' represent the same
+ // logical entity.
+ ASSERT_EQ(s_in.HashCode(), s_un.HashCode());
+ ASSERT_EQ(s_in, s_un);
+
+ Sockaddr s_in_0(s_in);
+ ASSERT_EQ(s_in.HashCode(), s_in_0.HashCode());
+ ASSERT_EQ(s_in, s_in_0);
+
+ Sockaddr s_in_1(s_un);
+ ASSERT_EQ(s_in.HashCode(), s_in_1.HashCode());
+ ASSERT_EQ(s_in, s_in_1);
+
+ Sockaddr s_un_0;
+ ASSERT_OK(s_un.ParseUnixDomainPath(kPath));
+ s_un_0 = s_in_0;
+ ASSERT_EQ(s_in.HashCode(), s_un_0.HashCode());
+ ASSERT_EQ(s_in_0, s_un_0);
+
+ Sockaddr s_un_1;
+ ASSERT_OK(s_un_1.ParseUnixDomainPath(kPath));
+ s_un_1 = s_in.ipv4_addr();
+ ASSERT_EQ(s_in.HashCode(), s_un_1.HashCode());
+ ASSERT_EQ(s_in, s_un_1);
+}
+
class SocketTest : public KuduTest {
protected:
Socket listener_;