From 3738c3afcbae664bb58455270a57a912517a329a Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 00:36:31 +0800 Subject: [PATCH 01/11] [BUG] Make SocketAddr string parsing memory safe SocketAddr(char const *) had one memory bug per platform. On Windows the copy loop was bounded by sizeof(buf), which is a byte count, while indexing a WCHAR array, so an address longer than 200 characters wrote up to 400 bytes past the end. It also assigned a possibly negative char straight to a WCHAR. On POSIX the host was copied into an uninitialized char[16] that was terminated at a fixed index, so inet_pton read indeterminate bytes for any host shorter than 15 characters. With a dirtied stack this silently resolves "1.2.3.4:80" to 0.0.0.0:80. Both now bound and terminate by what was actually copied, and the WSAStringToAddressW and inet_pton results are no longer discarded. Adds ext/test/http/socket_tools_test.cc, the first in-repo caller of this constructor. Fixes #4290 Fixes #4291 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/server/socket_tools.h | 32 ++++++--- ext/test/http/BUILD | 12 ++++ ext/test/http/CMakeLists.txt | 9 +++ ext/test/http/socket_tools_test.cc | 65 +++++++++++++++++++ 4 files changed, 110 insertions(+), 8 deletions(-) create mode 100644 ext/test/http/socket_tools_test.cc diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index 6a5b830f91..2bb5d1053c 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -186,12 +186,19 @@ struct SocketAddr #ifdef _WIN32 INT addrlen = sizeof(m_data); WCHAR buf[200]; - for (int i = 0; i < sizeof(buf) && addr[i]; i++) + // sizeof(buf) is a byte count, not an element count: bound the copy by the latter. + size_t const capacity = sizeof(buf) / sizeof(buf[0]) - 1; + size_t copied = 0; + while (copied < capacity && addr[copied]) { - buf[i] = addr[i]; + buf[copied] = static_cast(static_cast(addr[copied])); + ++copied; + } + buf[copied] = L'\0'; + if (::WSAStringToAddressW(buf, AF_INET, nullptr, &m_data, &addrlen) != 0) + { + LOG_WARN("SocketAddr: cannot parse address %s", addr); } - buf[199] = L'\0'; - ::WSAStringToAddressW(buf, AF_INET, nullptr, &m_data, &addrlen); #else sockaddr_in &inet4 = reinterpret_cast(m_data); inet4.sin_family = AF_INET; @@ -211,14 +218,23 @@ struct SocketAddr inet4.sin_port = 0; } char buf[16]; - memcpy(buf, addr, (std::min)(15, colon - addr)); - buf[15] = '\0'; - ::inet_pton(AF_INET, buf, &inet4.sin_addr); + // Terminate after what was actually copied: buf is uninitialized, so terminating at a + // fixed index would leave indeterminate bytes for inet_pton to read. + size_t const hostLength = static_cast((std::min)(15, colon - addr)); + memcpy(buf, addr, hostLength); + buf[hostLength] = '\0'; + if (::inet_pton(AF_INET, buf, &inet4.sin_addr) != 1) + { + LOG_WARN("SocketAddr: cannot parse address %s", addr); + } } else { inet4.sin_port = 0; - ::inet_pton(AF_INET, addr, &inet4.sin_addr); + if (::inet_pton(AF_INET, addr, &inet4.sin_addr) != 1) + { + LOG_WARN("SocketAddr: cannot parse address %s", addr); + } } #endif } diff --git a/ext/test/http/BUILD b/ext/test/http/BUILD index 2e6a07e688..696f71ee34 100644 --- a/ext/test/http/BUILD +++ b/ext/test/http/BUILD @@ -3,6 +3,18 @@ load("@rules_cc//cc:cc_test.bzl", "cc_test") +cc_test( + name = "socket_tools_test", + srcs = [ + "socket_tools_test.cc", + ], + tags = ["test"], + deps = [ + "//ext:headers", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "curl_http_test", srcs = [ diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index b7707bd8c2..122efd015e 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -14,6 +14,15 @@ if(WITH_HTTP_CLIENT_CURL) TEST_LIST ${FILENAME}) endif() +set(SOCKET_TOOLS_FILENAME socket_tools_test) +add_executable(${SOCKET_TOOLS_FILENAME} ${SOCKET_TOOLS_FILENAME}.cc) +target_link_libraries(${SOCKET_TOOLS_FILENAME} opentelemetry_ext ${GMOCK_LIB} + ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) +gtest_add_tests( + TARGET ${SOCKET_TOOLS_FILENAME} + TEST_PREFIX ext.http.sockettools. + TEST_LIST ${SOCKET_TOOLS_FILENAME}) + set(URL_PARSER_FILENAME url_parser_test) add_executable(${URL_PARSER_FILENAME} ${URL_PARSER_FILENAME}.cc) target_link_libraries(${URL_PARSER_FILENAME} opentelemetry_ext ${GMOCK_LIB} diff --git a/ext/test/http/socket_tools_test.cc b/ext/test/http/socket_tools_test.cc new file mode 100644 index 0000000000..2e15e28996 --- /dev/null +++ b/ext/test/http/socket_tools_test.cc @@ -0,0 +1,65 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include +#include + +#include "opentelemetry/ext/http/server/socket_tools.h" + +namespace +{ + +TEST(SocketAddrTest, ParsesHostAndPort) +{ + SocketTools::SocketAddr addr("127.0.0.1:8800"); + EXPECT_EQ(addr.port(), 8800); + EXPECT_EQ(addr.toString(), "127.0.0.1:8800"); +} + +// The host part used to be copied into an uninitialized buffer that was terminated at a +// fixed index, so inet_pton read indeterminate bytes for any host shorter than 15 +// characters. +TEST(SocketAddrTest, ParsesHostShorterThanTheBuffer) +{ + SocketTools::SocketAddr addr("1.2.3.4:80"); + EXPECT_EQ(addr.port(), 80); + EXPECT_EQ(addr.toString(), "1.2.3.4:80"); +} + +TEST(SocketAddrTest, ParsesLongestRepresentableHost) +{ + SocketTools::SocketAddr addr("255.255.255.255:65535"); + EXPECT_EQ(addr.port(), 65535); + EXPECT_EQ(addr.toString(), "255.255.255.255:65535"); +} + +TEST(SocketAddrTest, ParsesHostWithoutPort) +{ + SocketTools::SocketAddr addr("10.0.0.1"); + EXPECT_EQ(addr.port(), 0); + EXPECT_EQ(addr.toString(), "10.0.0.1:0"); +} + +TEST(SocketAddrTest, RejectsOutOfRangePort) +{ + SocketTools::SocketAddr addr("127.0.0.1:99999"); + EXPECT_EQ(addr.port(), 0); +} + +// On Windows the copy loop was bounded by sizeof(buf), a byte count, rather than by the +// element count of a WCHAR array, so an address longer than 200 characters wrote past the +// end of the buffer. +TEST(SocketAddrTest, HandlesOverlongInput) +{ + const std::string overlong(512, '9'); + SocketTools::SocketAddr addr(overlong.c_str()); + EXPECT_EQ(addr.port(), 0); +} + +TEST(SocketAddrTest, HandlesEmptyInput) +{ + SocketTools::SocketAddr addr(""); + EXPECT_EQ(addr.port(), 0); +} + +} // namespace From fb21f08b9cec68bd4894a690aae0e449f9aba516 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 00:37:54 +0800 Subject: [PATCH 02/11] Add CHANGELOG entry for the SocketAddr parsing fix Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8baba4503c..6da0a794cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ Increment the: namespaces [#4303](https://github.com/open-telemetry/opentelemetry-cpp/pull/4303) +* [BUG] Make SocketAddr string parsing memory safe on Windows and POSIX + [#4292](https://github.com/open-telemetry/opentelemetry-cpp/pull/4292) + * [CODE HEALTH] Move metrics storage test fixtures into anonymous namespace [#4286](https://github.com/open-telemetry/opentelemetry-cpp/pull/4286) From 00443cb7c0a8dcfd3f77e26d80e0651f0c505ab5 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 11:51:21 +0800 Subject: [PATCH 03/11] Give SocketAddr a valid/invalid contract and fix the Windows parser Addresses review of #4292. Windows: WSAStringToAddress fails with WSAEINVAL unless the sockaddr's family is preset, so the old parser left AF_UNSPEC and every address failed to parse. sin_family is now set before the call. Windows CI is build-only, so this path was compiled but never run. A failed parse now leaves an unusable address (AF_UNSPEC, port() == -1) instead of a plausible fallback that connect()/bind() would accept. That lets callers tell a parse failure from a real endpoint, including the legitimate ":0". POSIX host handling rejects a host longer than the buffer instead of truncating it into a different valid address ("255.255.255.2559" no longer becomes "255.255.255.255"), and the port parse now requires the whole string to be consumed, so ":80junk" and ":80:90" are rejected rather than silently truncated to the leading number. Tests assert port() == -1 for rejection rather than == 0, which could not tell a failure from a valid ":0", and cover the truncation, trailing-garbage and invalid-host cases the earlier tests missed. The test target links ws2_32 explicitly on Windows so it also builds under MinGW/GCC, where the header's #pragma comment(lib) autolink is ignored. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/server/socket_tools.h | 70 ++++++++++++------- ext/test/http/BUILD | 4 ++ ext/test/http/CMakeLists.txt | 5 ++ ext/test/http/socket_tools_test.cc | 49 ++++++++++--- 4 files changed, 95 insertions(+), 33 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index 2bb5d1053c..a84bd8b640 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -184,7 +184,9 @@ struct SocketAddr SocketAddr(char const *addr) { #ifdef _WIN32 - INT addrlen = sizeof(m_data); + // WSAStringToAddress fails with WSAEINVAL unless the sockaddr's family is preset. + m_data.sa_family = AF_INET; + INT addrlen = sizeof(m_data); WCHAR buf[200]; // sizeof(buf) is a byte count, not an element count: bound the copy by the latter. size_t const capacity = sizeof(buf) / sizeof(buf[0]) - 1; @@ -197,44 +199,64 @@ struct SocketAddr buf[copied] = L'\0'; if (::WSAStringToAddressW(buf, AF_INET, nullptr, &m_data, &addrlen) != 0) { + // Leave an unusable address rather than a plausible endpoint that connect()/bind() would + // accept: a family of AF_UNSPEC makes port() return -1. + m_data = {}; LOG_WARN("SocketAddr: cannot parse address %s", addr); } #else sockaddr_in &inet4 = reinterpret_cast(m_data); inet4.sin_family = AF_INET; - char const *colon = strchr(addr, ':'); - if (colon) + + char const *colon = strchr(addr, ':'); + char const *hostEnd = colon ? colon : addr + strlen(addr); + ptrdiff_t const hostLength = hostEnd - addr; + + bool ok = true; + + // Reject a host that would not fit rather than truncating it into a different valid address + // (for example "255.255.255.2559" would otherwise become "255.255.255.255"). + if (hostLength <= 0 || hostLength > 15) + { + ok = false; + } + else + { + char host[16]; + memcpy(host, addr, static_cast(hostLength)); + host[hostLength] = '\0'; + ok = (::inet_pton(AF_INET, host, &inet4.sin_addr) == 1); + } + + uint16_t port = 0; + if (ok && colon) { - char *portEnd = nullptr; - errno = 0; + char *portEnd = nullptr; + errno = 0; + // auto keeps strtol's long return type without tripping google-runtime-int. auto const parsed = std::strtol(colon + 1, &portEnd, 10); - // Accept only a converted, in-range port value; fall back to 0 otherwise. - if (errno == 0 && portEnd != colon + 1 && parsed >= 0 && parsed <= 65535) + // Require the entire port to be consumed, so trailing text such as ":80junk" or a second + // ":90" is rejected rather than silently truncated to the leading number. + if (errno != 0 || portEnd == colon + 1 || *portEnd != '\0' || parsed < 0 || parsed > 65535) { - inet4.sin_port = htons(static_cast(parsed)); + ok = false; } else { - inet4.sin_port = 0; - } - char buf[16]; - // Terminate after what was actually copied: buf is uninitialized, so terminating at a - // fixed index would leave indeterminate bytes for inet_pton to read. - size_t const hostLength = static_cast((std::min)(15, colon - addr)); - memcpy(buf, addr, hostLength); - buf[hostLength] = '\0'; - if (::inet_pton(AF_INET, buf, &inet4.sin_addr) != 1) - { - LOG_WARN("SocketAddr: cannot parse address %s", addr); + port = static_cast(parsed); } } + + if (ok) + { + inet4.sin_port = htons(port); + } else { - inet4.sin_port = 0; - if (::inet_pton(AF_INET, addr, &inet4.sin_addr) != 1) - { - LOG_WARN("SocketAddr: cannot parse address %s", addr); - } + // Unusable address; port() returns -1 so callers can tell parse failure from a real + // endpoint, including the legitimate ":0". + m_data = {}; + LOG_WARN("SocketAddr: cannot parse address %s", addr); } #endif } diff --git a/ext/test/http/BUILD b/ext/test/http/BUILD index 696f71ee34..806820711a 100644 --- a/ext/test/http/BUILD +++ b/ext/test/http/BUILD @@ -8,6 +8,10 @@ cc_test( srcs = [ "socket_tools_test.cc", ], + linkopts = select({ + "//bazel:windows": ["-DEFAULTLIB:ws2_32.lib"], + "//conditions:default": [], + }), tags = ["test"], deps = [ "//ext:headers", diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index 122efd015e..5cce56f917 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -18,6 +18,11 @@ set(SOCKET_TOOLS_FILENAME socket_tools_test) add_executable(${SOCKET_TOOLS_FILENAME} ${SOCKET_TOOLS_FILENAME}.cc) target_link_libraries(${SOCKET_TOOLS_FILENAME} opentelemetry_ext ${GMOCK_LIB} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) +if(WIN32) + # socket_tools.h autolinks ws2_32 via #pragma comment(lib) under MSVC only; name it + # explicitly so the test also links under MinGW/GCC where that pragma is ignored. + target_link_libraries(${SOCKET_TOOLS_FILENAME} ws2_32) +endif() gtest_add_tests( TARGET ${SOCKET_TOOLS_FILENAME} TEST_PREFIX ext.http.sockettools. diff --git a/ext/test/http/socket_tools_test.cc b/ext/test/http/socket_tools_test.cc index 2e15e28996..d0c69f465a 100644 --- a/ext/test/http/socket_tools_test.cc +++ b/ext/test/http/socket_tools_test.cc @@ -9,6 +9,9 @@ namespace { +// A parsed address reports a family and a non-negative port; a rejected one reports port() == -1 +// (its family is left AF_UNSPEC), which is how callers tell a real endpoint from a parse failure. + TEST(SocketAddrTest, ParsesHostAndPort) { SocketTools::SocketAddr addr("127.0.0.1:8800"); @@ -16,9 +19,8 @@ TEST(SocketAddrTest, ParsesHostAndPort) EXPECT_EQ(addr.toString(), "127.0.0.1:8800"); } -// The host part used to be copied into an uninitialized buffer that was terminated at a -// fixed index, so inet_pton read indeterminate bytes for any host shorter than 15 -// characters. +// The host part used to be copied into an uninitialized buffer that was terminated at a fixed +// index, so inet_pton read indeterminate bytes for any host shorter than 15 characters. TEST(SocketAddrTest, ParsesHostShorterThanTheBuffer) { SocketTools::SocketAddr addr("1.2.3.4:80"); @@ -40,26 +42,55 @@ TEST(SocketAddrTest, ParsesHostWithoutPort) EXPECT_EQ(addr.toString(), "10.0.0.1:0"); } +// A legitimate port 0 must be distinguishable from a parse failure, so this must parse rather +// than collapse to the same state the rejections use. +TEST(SocketAddrTest, ParsesLegitimateZeroPort) +{ + SocketTools::SocketAddr addr("127.0.0.1:0"); + EXPECT_EQ(addr.port(), 0); + EXPECT_EQ(addr.toString(), "127.0.0.1:0"); +} + TEST(SocketAddrTest, RejectsOutOfRangePort) { SocketTools::SocketAddr addr("127.0.0.1:99999"); - EXPECT_EQ(addr.port(), 0); + EXPECT_EQ(addr.port(), -1); +} + +// A host longer than the buffer must be rejected, not truncated into a different valid address: +// "255.255.255.2559" must not become "255.255.255.255". +TEST(SocketAddrTest, RejectsOverlongHostInsteadOfTruncating) +{ + SocketTools::SocketAddr addr("255.255.255.2559:80"); + EXPECT_EQ(addr.port(), -1); } -// On Windows the copy loop was bounded by sizeof(buf), a byte count, rather than by the -// element count of a WCHAR array, so an address longer than 200 characters wrote past the -// end of the buffer. +TEST(SocketAddrTest, RejectsTrailingGarbageInPort) +{ + EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:80junk").port(), -1); + EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:80:90").port(), -1); +} + +TEST(SocketAddrTest, RejectsInvalidHost) +{ + EXPECT_EQ(SocketTools::SocketAddr("999.999.999.999:4318").port(), -1); + EXPECT_EQ(SocketTools::SocketAddr("garbage").port(), -1); +} + +// On Windows the copy loop was bounded by sizeof(buf), a byte count, rather than by the element +// count of a WCHAR array, so an address longer than 200 characters wrote past the end of the +// buffer. This must be handled rather than crash, and reported as a rejection. TEST(SocketAddrTest, HandlesOverlongInput) { const std::string overlong(512, '9'); SocketTools::SocketAddr addr(overlong.c_str()); - EXPECT_EQ(addr.port(), 0); + EXPECT_EQ(addr.port(), -1); } TEST(SocketAddrTest, HandlesEmptyInput) { SocketTools::SocketAddr addr(""); - EXPECT_EQ(addr.port(), 0); + EXPECT_EQ(addr.port(), -1); } } // namespace From fc582f9be7381d7e87f37aea44d292d15d986a79 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 11:56:12 +0800 Subject: [PATCH 04/11] Match the CHANGELOG wording to the widened SocketAddr fix Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6da0a794cd..835267d330 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,7 @@ Increment the: namespaces [#4303](https://github.com/open-telemetry/opentelemetry-cpp/pull/4303) -* [BUG] Make SocketAddr string parsing memory safe on Windows and POSIX +* [BUG] Make SocketAddr string parsing safe and reject malformed addresses [#4292](https://github.com/open-telemetry/opentelemetry-cpp/pull/4292) * [CODE HEALTH] Move metrics storage test fixtures into anonymous namespace From c0b1595815d4b98d568721f872c5a5130ae0e77c Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 12:36:38 +0800 Subject: [PATCH 05/11] Use one inet_pton-based parser for SocketAddr on both platforms Second review round. The Windows and POSIX paths were different parsers, so they could not guarantee the same grammar: WSAStringToAddress fills missing components with defaults and its provider grammar differs from the POSIX split, and the Windows copy loop also truncated input over 199 characters before parsing it. Both platforms now share one parser: a null check, inet_pton for the host (Winsock has provided it since Vista, via ), and a decimal-only port scan that rejects the sign and whitespace strtol would have accepted and checks overflow before it can wrap. The result is parsed into a local sockaddr_in and committed with memcpy only on success, so a failure leaves AF_UNSPEC and the storage is never accessed through a misaligned sockaddr_in glvalue. port() and toString() copy out the same way, which removes the type-access/alignment UB there too (confirmed with UBSan at a 2-mod-4 address); the storage type is unchanged, so this is ABI preserving. Because the parser is now platform independent it is fully exercised by the Linux test run rather than only compiled on Windows. New tests cover null, empty host/port, sign and whitespace, overflow, and non-dotted-quad hosts (127.1, leading zeros). The integer SocketAddr(u_long, int) overload still narrows an out-of-range port; that has a real caller (addListeningPort) so it is left for its own issue rather than widened into this change. Fixes #4290 Fixes #4291 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/server/socket_tools.h | 104 +++++++++--------- ext/test/http/socket_tools_test.cc | 35 ++++++ 2 files changed, 88 insertions(+), 51 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index a84bd8b640..0b4ac6ed37 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -22,6 +22,7 @@ // # include # include +# include // inet_pton // TODO: consider NOMINMAX # undef min @@ -183,82 +184,79 @@ struct SocketAddr SocketAddr(char const *addr) { -#ifdef _WIN32 - // WSAStringToAddress fails with WSAEINVAL unless the sockaddr's family is preset. - m_data.sa_family = AF_INET; - INT addrlen = sizeof(m_data); - WCHAR buf[200]; - // sizeof(buf) is a byte count, not an element count: bound the copy by the latter. - size_t const capacity = sizeof(buf) / sizeof(buf[0]) - 1; - size_t copied = 0; - while (copied < capacity && addr[copied]) - { - buf[copied] = static_cast(static_cast(addr[copied])); - ++copied; - } - buf[copied] = L'\0'; - if (::WSAStringToAddressW(buf, AF_INET, nullptr, &m_data, &addrlen) != 0) + // One parser for every platform: inet_pton (Winsock provides it since Vista) plus a strict + // decimal port. This avoids WSAStringToAddress, whose grammar and default-component filling + // differ from the POSIX path. Parse into a local sockaddr_in and commit with memcpy only on + // success, which keeps m_data at AF_UNSPEC on failure and avoids accessing the sockaddr + // storage through a sockaddr_in glvalue (an alignment/type-access issue tracked in #4287). + if (addr == nullptr) { - // Leave an unusable address rather than a plausible endpoint that connect()/bind() would - // accept: a family of AF_UNSPEC makes port() return -1. - m_data = {}; - LOG_WARN("SocketAddr: cannot parse address %s", addr); + LOG_WARN("SocketAddr: cannot parse a null address"); + return; // m_data is already AF_UNSPEC, so port() reports -1. } -#else - sockaddr_in &inet4 = reinterpret_cast(m_data); - inet4.sin_family = AF_INET; + + sockaddr_in parsed{}; + parsed.sin_family = AF_INET; char const *colon = strchr(addr, ':'); char const *hostEnd = colon ? colon : addr + strlen(addr); ptrdiff_t const hostLength = hostEnd - addr; - bool ok = true; - // Reject a host that would not fit rather than truncating it into a different valid address - // (for example "255.255.255.2559" would otherwise become "255.255.255.255"). - if (hostLength <= 0 || hostLength > 15) - { - ok = false; - } - else + // (for example "255.255.255.2559" would otherwise become "255.255.255.255"). No dotted-quad + // exceeds 15 characters. + bool ok = (hostLength >= 1 && hostLength <= 15); + if (ok) { char host[16]; memcpy(host, addr, static_cast(hostLength)); host[hostLength] = '\0'; - ok = (::inet_pton(AF_INET, host, &inet4.sin_addr) == 1); + ok = (::inet_pton(AF_INET, host, &parsed.sin_addr) == 1); } - uint16_t port = 0; + // Port: decimal digits only, with overflow checked before it can wrap. strtol would also + // accept a leading sign or whitespace and depend on the locale. if (ok && colon) { - char *portEnd = nullptr; - errno = 0; - // auto keeps strtol's long return type without tripping google-runtime-int. - auto const parsed = std::strtol(colon + 1, &portEnd, 10); - // Require the entire port to be consumed, so trailing text such as ":80junk" or a second - // ":90" is rejected rather than silently truncated to the leading number. - if (errno != 0 || portEnd == colon + 1 || *portEnd != '\0' || parsed < 0 || parsed > 65535) + char const *p = colon + 1; + unsigned int port = 0; + if (*p == '\0') { - ok = false; + ok = false; // empty port, e.g. "127.0.0.1:" } - else + while (ok && *p != '\0') { - port = static_cast(parsed); + if (*p < '0' || *p > '9') + { + ok = false; + break; + } + unsigned int const digit = static_cast(*p - '0'); + if (port > (65535u - digit) / 10u) + { + ok = false; // would exceed 65535 + break; + } + port = port * 10u + digit; + ++p; + } + if (ok) + { + parsed.sin_port = htons(static_cast(port)); } } if (ok) { - inet4.sin_port = htons(port); + memcpy(&m_data, &parsed, sizeof(parsed)); } else { - // Unusable address; port() returns -1 so callers can tell parse failure from a real - // endpoint, including the legitimate ":0". - m_data = {}; - LOG_WARN("SocketAddr: cannot parse address %s", addr); + // Leave m_data at AF_UNSPEC; port() returns -1 so callers can tell a parse failure from a + // real endpoint, including the legitimate ":0". Do not echo the raw input, which may be + // arbitrarily long. + LOG_WARN("SocketAddr: cannot parse address"); } -#endif } operator sockaddr *() { return &m_data; } @@ -270,7 +268,10 @@ struct SocketAddr switch (m_data.sa_family) { case AF_INET: { - sockaddr_in const &inet4 = reinterpret_cast(m_data); + // Copy out rather than binding a sockaddr_in glvalue to sockaddr storage, which is an + // alignment/type-access issue (see the constructor and #4287). + sockaddr_in inet4{}; + memcpy(&inet4, &m_data, sizeof(inet4)); return ntohs(inet4.sin_port); } @@ -286,8 +287,9 @@ struct SocketAddr switch (m_data.sa_family) { case AF_INET: { - sockaddr_in const &inet4 = reinterpret_cast(m_data); - u_long addr = ntohl(inet4.sin_addr.s_addr); + sockaddr_in inet4{}; + memcpy(&inet4, &m_data, sizeof(inet4)); + u_long addr = ntohl(inet4.sin_addr.s_addr); os << (addr >> 24) << '.' << ((addr >> 16) & 255) << '.' << ((addr >> 8) & 255) << '.' << (addr & 255); os << ':' << ntohs(inet4.sin_port); diff --git a/ext/test/http/socket_tools_test.cc b/ext/test/http/socket_tools_test.cc index d0c69f465a..fce608085f 100644 --- a/ext/test/http/socket_tools_test.cc +++ b/ext/test/http/socket_tools_test.cc @@ -93,4 +93,39 @@ TEST(SocketAddrTest, HandlesEmptyInput) EXPECT_EQ(addr.port(), -1); } +TEST(SocketAddrTest, RejectsNullInput) +{ + SocketTools::SocketAddr addr(nullptr); + EXPECT_EQ(addr.port(), -1); +} + +TEST(SocketAddrTest, RejectsEmptyHostOrPort) +{ + EXPECT_EQ(SocketTools::SocketAddr(":80").port(), -1); + EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:").port(), -1); +} + +// The port grammar is decimal digits only: a sign or whitespace that strtol would have accepted +// must be rejected so both platforms agree. +TEST(SocketAddrTest, RejectsSignAndWhitespaceInPort) +{ + EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:+80").port(), -1); + EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:-0").port(), -1); + EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1: 80").port(), -1); +} + +TEST(SocketAddrTest, RejectsPortOverflow) +{ + EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:65536").port(), -1); + EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:4294967377").port(), -1); +} + +// inet_pton requires a full dotted quad, so shorthand and leading-zero forms are rejected on +// every platform rather than parsed differently by a legacy resolver. +TEST(SocketAddrTest, RejectsNonDottedQuadHost) +{ + EXPECT_EQ(SocketTools::SocketAddr("127.1:80").port(), -1); + EXPECT_EQ(SocketTools::SocketAddr("01.02.03.004:80").port(), -1); +} + } // namespace From 077bab60b3d9e68da1783379bbd0e22f39d5c28d Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 15:16:10 +0800 Subject: [PATCH 06/11] Pin SocketAddr storage layout and harden its parser tests The char* parser memcpys a sockaddr_in into the sockaddr storage and the socket syscalls pass sizeof(SocketAddr) as the length, both of which assume sockaddr and sockaddr_in share a size and family offset. Make those assumptions static_asserts so a hostile ABI fails to compile rather than corrupting memory at run time. Assert the address family in the tests, not just port(): a rejected input must stay AF_UNSPEC and a legitimate ":0" must parse as AF_INET, so the two states can never be confused by a stray family byte. The negative assertions move into an ExpectInvalid helper, and the non-dotted-quad comment now scopes its "rejected everywhere" claim to the BIND-derived inet_pton parsers (glibc, musl, macOS, Winsock) that were verified to reject the shorthand and leading-zero forms. Declare ws2_32 as an interface dependency of opentelemetry_ext in both CMake and Bazel so consumers and the test inherit it, instead of each test target relinking it and papering over the public target's missing usage requirement. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/BUILD | 5 ++ ext/CMakeLists.txt | 7 +++ .../ext/http/server/socket_tools.h | 10 ++++ ext/test/http/CMakeLists.txt | 7 +-- ext/test/http/socket_tools_test.cc | 53 +++++++++++-------- 5 files changed, 55 insertions(+), 27 deletions(-) diff --git a/ext/BUILD b/ext/BUILD index 52ed22afb5..82ff180a0e 100644 --- a/ext/BUILD +++ b/ext/BUILD @@ -8,5 +8,10 @@ package(default_visibility = ["//visibility:public"]) cc_library( name = "headers", hdrs = glob(["include/**/*.h"]), + linkopts = select({ + # socket_tools.h calls Winsock; declare it on the interface so consumers link it too. + "//bazel:windows": ["-DEFAULTLIB:Ws2_32.lib"], + "//conditions:default": [], + }), strip_include_prefix = "include", ) diff --git a/ext/CMakeLists.txt b/ext/CMakeLists.txt index d50cf3a759..cd59c9cba7 100644 --- a/ext/CMakeLists.txt +++ b/ext/CMakeLists.txt @@ -10,6 +10,13 @@ target_include_directories( set_target_properties(opentelemetry_ext PROPERTIES EXPORT_NAME "ext") target_link_libraries(opentelemetry_ext INTERFACE opentelemetry_api) +# The embedded HTTP server headers (socket_tools.h) call Winsock, so declare the dependency on +# the interface target. MSVC also autolinks via #pragma comment(lib), but MinGW/GCC ignore that, +# so consumers there would otherwise fail to resolve inet_pton/WSAStartup. +if(WIN32) + target_link_libraries(opentelemetry_ext INTERFACE ws2_32) +endif() + otel_add_component( COMPONENT ext_common diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index 0b4ac6ed37..377a060aec 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -303,6 +303,16 @@ struct SocketAddr } }; +// The parser memcpys a sockaddr_in into m_data, and the socket syscalls pass sizeof(SocketAddr) +// as the address length. Make those assumptions compile-time guarantees rather than trusting +// every ABI to keep sockaddr and sockaddr_in the same size and family offset. +static_assert(sizeof(sockaddr) >= sizeof(sockaddr_in), + "SocketAddr storage (sockaddr) must be large enough to hold a sockaddr_in"); +static_assert(offsetof(sockaddr, sa_family) == offsetof(sockaddr_in, sin_family), + "sockaddr and sockaddr_in must place the address family at the same offset"); +static_assert(sizeof(SocketAddr) == sizeof(sockaddr), + "SocketAddr must add no storage beyond its sockaddr, since syscalls use its size"); + /// /// Encapsulation of a socket (non-exclusive ownership) /// diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index 5cce56f917..0675cff8ee 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -18,11 +18,8 @@ set(SOCKET_TOOLS_FILENAME socket_tools_test) add_executable(${SOCKET_TOOLS_FILENAME} ${SOCKET_TOOLS_FILENAME}.cc) target_link_libraries(${SOCKET_TOOLS_FILENAME} opentelemetry_ext ${GMOCK_LIB} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) -if(WIN32) - # socket_tools.h autolinks ws2_32 via #pragma comment(lib) under MSVC only; name it - # explicitly so the test also links under MinGW/GCC where that pragma is ignored. - target_link_libraries(${SOCKET_TOOLS_FILENAME} ws2_32) -endif() +# ws2_32 is now an interface dependency of opentelemetry_ext, so it is inherited here; the test +# thus verifies the public target's usage requirements rather than papering over them locally. gtest_add_tests( TARGET ${SOCKET_TOOLS_FILENAME} TEST_PREFIX ext.http.sockettools. diff --git a/ext/test/http/socket_tools_test.cc b/ext/test/http/socket_tools_test.cc index fce608085f..9e088313d1 100644 --- a/ext/test/http/socket_tools_test.cc +++ b/ext/test/http/socket_tools_test.cc @@ -9,12 +9,19 @@ namespace { -// A parsed address reports a family and a non-negative port; a rejected one reports port() == -1 -// (its family is left AF_UNSPEC), which is how callers tell a real endpoint from a parse failure. +// A parsed address reports AF_INET and a non-negative port; a rejected one is left at AF_UNSPEC +// with port() == -1. Checking the family too, not just the port, catches a family byte being +// corrupted while port() happens to still return -1. +void ExpectInvalid(const SocketTools::SocketAddr &addr) +{ + EXPECT_EQ(addr.m_data.sa_family, AF_UNSPEC); + EXPECT_EQ(addr.port(), -1); +} TEST(SocketAddrTest, ParsesHostAndPort) { SocketTools::SocketAddr addr("127.0.0.1:8800"); + EXPECT_EQ(addr.m_data.sa_family, AF_INET); EXPECT_EQ(addr.port(), 8800); EXPECT_EQ(addr.toString(), "127.0.0.1:8800"); } @@ -47,6 +54,7 @@ TEST(SocketAddrTest, ParsesHostWithoutPort) TEST(SocketAddrTest, ParsesLegitimateZeroPort) { SocketTools::SocketAddr addr("127.0.0.1:0"); + EXPECT_EQ(addr.m_data.sa_family, AF_INET); EXPECT_EQ(addr.port(), 0); EXPECT_EQ(addr.toString(), "127.0.0.1:0"); } @@ -54,7 +62,7 @@ TEST(SocketAddrTest, ParsesLegitimateZeroPort) TEST(SocketAddrTest, RejectsOutOfRangePort) { SocketTools::SocketAddr addr("127.0.0.1:99999"); - EXPECT_EQ(addr.port(), -1); + ExpectInvalid(addr); } // A host longer than the buffer must be rejected, not truncated into a different valid address: @@ -62,19 +70,19 @@ TEST(SocketAddrTest, RejectsOutOfRangePort) TEST(SocketAddrTest, RejectsOverlongHostInsteadOfTruncating) { SocketTools::SocketAddr addr("255.255.255.2559:80"); - EXPECT_EQ(addr.port(), -1); + ExpectInvalid(addr); } TEST(SocketAddrTest, RejectsTrailingGarbageInPort) { - EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:80junk").port(), -1); - EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:80:90").port(), -1); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:80junk")); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:80:90")); } TEST(SocketAddrTest, RejectsInvalidHost) { - EXPECT_EQ(SocketTools::SocketAddr("999.999.999.999:4318").port(), -1); - EXPECT_EQ(SocketTools::SocketAddr("garbage").port(), -1); + ExpectInvalid(SocketTools::SocketAddr("999.999.999.999:4318")); + ExpectInvalid(SocketTools::SocketAddr("garbage")); } // On Windows the copy loop was bounded by sizeof(buf), a byte count, rather than by the element @@ -84,48 +92,49 @@ TEST(SocketAddrTest, HandlesOverlongInput) { const std::string overlong(512, '9'); SocketTools::SocketAddr addr(overlong.c_str()); - EXPECT_EQ(addr.port(), -1); + ExpectInvalid(addr); } TEST(SocketAddrTest, HandlesEmptyInput) { SocketTools::SocketAddr addr(""); - EXPECT_EQ(addr.port(), -1); + ExpectInvalid(addr); } TEST(SocketAddrTest, RejectsNullInput) { SocketTools::SocketAddr addr(nullptr); - EXPECT_EQ(addr.port(), -1); + ExpectInvalid(addr); } TEST(SocketAddrTest, RejectsEmptyHostOrPort) { - EXPECT_EQ(SocketTools::SocketAddr(":80").port(), -1); - EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:").port(), -1); + ExpectInvalid(SocketTools::SocketAddr(":80")); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:")); } // The port grammar is decimal digits only: a sign or whitespace that strtol would have accepted // must be rejected so both platforms agree. TEST(SocketAddrTest, RejectsSignAndWhitespaceInPort) { - EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:+80").port(), -1); - EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:-0").port(), -1); - EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1: 80").port(), -1); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:+80")); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:-0")); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1: 80")); } TEST(SocketAddrTest, RejectsPortOverflow) { - EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:65536").port(), -1); - EXPECT_EQ(SocketTools::SocketAddr("127.0.0.1:4294967377").port(), -1); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:65536")); + ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:4294967377")); } -// inet_pton requires a full dotted quad, so shorthand and leading-zero forms are rejected on -// every platform rather than parsed differently by a legacy resolver. +// inet_pton() rejects the shorthand (127.1) and leading-zero (01.02.03.004) forms that the legacy +// inet_aton()/inet_addr() resolvers accepted, on the BIND-derived parsers used by glibc, musl, +// macOS, and Winsock. TEST(SocketAddrTest, RejectsNonDottedQuadHost) { - EXPECT_EQ(SocketTools::SocketAddr("127.1:80").port(), -1); - EXPECT_EQ(SocketTools::SocketAddr("01.02.03.004:80").port(), -1); + ExpectInvalid(SocketTools::SocketAddr("127.1:80")); + ExpectInvalid(SocketTools::SocketAddr("01.02.03.004:80")); } } // namespace From 7d413130d7745fe8bf03107d57f3a08be454af64 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:41:58 +0800 Subject: [PATCH 07/11] Tighten the size invariant, drop a Bazel relink, and fix formatting Require sockaddr and sockaddr_in to have identical size, not just sockaddr large enough: with sizeof(SocketAddr) == sizeof(sockaddr), the length connect()/bind() pass is then exactly sizeof(sockaddr_in), which avoids a too-large address length that POSIX may reject with EINVAL. Drop the redundant ws2_32 linkopt from the Bazel socket_tools_test; it is inherited from //ext:headers, so the test now verifies the public target's usage requirements rather than relinking. Drop the unverifiable "BIND-derived" lineage claim from the non-dotted-quad test comment and just state inet_pton's canonical-form contract, and add a test showing a leading-zero port ("080") parses as 80. Reflow the two CMakeLists comments to satisfy cmake-format. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/CMakeLists.txt | 7 ++++--- .../opentelemetry/ext/http/server/socket_tools.h | 10 ++++++---- ext/test/http/BUILD | 6 ++---- ext/test/http/CMakeLists.txt | 5 +++-- ext/test/http/socket_tools_test.cc | 14 +++++++++++--- 5 files changed, 26 insertions(+), 16 deletions(-) diff --git a/ext/CMakeLists.txt b/ext/CMakeLists.txt index cd59c9cba7..39a97c4dea 100644 --- a/ext/CMakeLists.txt +++ b/ext/CMakeLists.txt @@ -10,9 +10,10 @@ target_include_directories( set_target_properties(opentelemetry_ext PROPERTIES EXPORT_NAME "ext") target_link_libraries(opentelemetry_ext INTERFACE opentelemetry_api) -# The embedded HTTP server headers (socket_tools.h) call Winsock, so declare the dependency on -# the interface target. MSVC also autolinks via #pragma comment(lib), but MinGW/GCC ignore that, -# so consumers there would otherwise fail to resolve inet_pton/WSAStartup. +# The embedded HTTP server headers (socket_tools.h) call Winsock, so declare the +# dependency on the interface target. MSVC also autolinks via #pragma +# comment(lib), but MinGW/GCC ignore that, so consumers there would otherwise +# fail to resolve inet_pton/WSAStartup. if(WIN32) target_link_libraries(opentelemetry_ext INTERFACE ws2_32) endif() diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index 377a060aec..162a0c7456 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -304,10 +304,12 @@ struct SocketAddr }; // The parser memcpys a sockaddr_in into m_data, and the socket syscalls pass sizeof(SocketAddr) -// as the address length. Make those assumptions compile-time guarantees rather than trusting -// every ABI to keep sockaddr and sockaddr_in the same size and family offset. -static_assert(sizeof(sockaddr) >= sizeof(sockaddr_in), - "SocketAddr storage (sockaddr) must be large enough to hold a sockaddr_in"); +// as the address length. This wrapper is IPv4-only, so require sockaddr and sockaddr_in to have +// the exact same size rather than trusting every ABI: passing an address length that is too large +// for the family is a documented EINVAL for connect()/bind(). Exact equality also keeps the memcpy +// safe. Together with the assertion below, sizeof(SocketAddr) == sizeof(sockaddr_in). +static_assert(sizeof(sockaddr) == sizeof(sockaddr_in), + "SocketAddr is IPv4-only: sockaddr and sockaddr_in must have identical size"); static_assert(offsetof(sockaddr, sa_family) == offsetof(sockaddr_in, sin_family), "sockaddr and sockaddr_in must place the address family at the same offset"); static_assert(sizeof(SocketAddr) == sizeof(sockaddr), diff --git a/ext/test/http/BUILD b/ext/test/http/BUILD index 806820711a..a94a2d19e3 100644 --- a/ext/test/http/BUILD +++ b/ext/test/http/BUILD @@ -8,10 +8,8 @@ cc_test( srcs = [ "socket_tools_test.cc", ], - linkopts = select({ - "//bazel:windows": ["-DEFAULTLIB:ws2_32.lib"], - "//conditions:default": [], - }), + # ws2_32 is an interface linkopt of //ext:headers, so it is inherited here rather than + # relinked, which also lets the test verify the public target's usage requirements. tags = ["test"], deps = [ "//ext:headers", diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index 0675cff8ee..14d6aee613 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -18,8 +18,9 @@ set(SOCKET_TOOLS_FILENAME socket_tools_test) add_executable(${SOCKET_TOOLS_FILENAME} ${SOCKET_TOOLS_FILENAME}.cc) target_link_libraries(${SOCKET_TOOLS_FILENAME} opentelemetry_ext ${GMOCK_LIB} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) -# ws2_32 is now an interface dependency of opentelemetry_ext, so it is inherited here; the test -# thus verifies the public target's usage requirements rather than papering over them locally. +# ws2_32 is now an interface dependency of opentelemetry_ext, so it is inherited +# here; the test thus verifies the public target's usage requirements rather +# than papering over them locally. gtest_add_tests( TARGET ${SOCKET_TOOLS_FILENAME} TEST_PREFIX ext.http.sockettools. diff --git a/ext/test/http/socket_tools_test.cc b/ext/test/http/socket_tools_test.cc index 9e088313d1..41f9fa45f1 100644 --- a/ext/test/http/socket_tools_test.cc +++ b/ext/test/http/socket_tools_test.cc @@ -59,6 +59,15 @@ TEST(SocketAddrTest, ParsesLegitimateZeroPort) EXPECT_EQ(addr.toString(), "127.0.0.1:0"); } +// A decimal port may carry leading zeros; the value is what matters, so "080" is port 80. The +// pre-multiply overflow check still bounds the value at 65535 no matter how many zeros precede it. +TEST(SocketAddrTest, AcceptsLeadingZeroPort) +{ + SocketTools::SocketAddr addr("127.0.0.1:080"); + EXPECT_EQ(addr.m_data.sa_family, AF_INET); + EXPECT_EQ(addr.port(), 80); +} + TEST(SocketAddrTest, RejectsOutOfRangePort) { SocketTools::SocketAddr addr("127.0.0.1:99999"); @@ -128,9 +137,8 @@ TEST(SocketAddrTest, RejectsPortOverflow) ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:4294967377")); } -// inet_pton() rejects the shorthand (127.1) and leading-zero (01.02.03.004) forms that the legacy -// inet_aton()/inet_addr() resolvers accepted, on the BIND-derived parsers used by glibc, musl, -// macOS, and Winsock. +// inet_pton() requires a canonical four-octet address, so it rejects the shorthand (127.1) and +// leading-zero (01.02.03.004) forms that the legacy inet_aton()/inet_addr() resolvers accepted. TEST(SocketAddrTest, RejectsNonDottedQuadHost) { ExpectInvalid(SocketTools::SocketAddr("127.1:80")); From dc3149f626ff3a86e253b7231c775dcf1fe9eea0 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 23:43:23 +0800 Subject: [PATCH 08/11] Fix CI: remove non-portable inet_pton test, add sys/socket.h for IWYU RejectsNonDottedQuadHost asserted inet_pton rejects shorthand (127.1) and leading-zero (01.02.03.004) hosts. That is not portable: glibc rejects them but macOS accepts them, so the test failed on the macOS runners. The memory-safety fixes stay covered by the buffer-length and overlong-host tests. Add (POSIX-guarded) for the sockaddr and AF_UNSPEC the helper names directly, as IWYU requires. --- ext/test/http/socket_tools_test.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/ext/test/http/socket_tools_test.cc b/ext/test/http/socket_tools_test.cc index 41f9fa45f1..08f7e2235d 100644 --- a/ext/test/http/socket_tools_test.cc +++ b/ext/test/http/socket_tools_test.cc @@ -3,6 +3,9 @@ #include #include +#ifndef _WIN32 +# include // for sockaddr, AF_UNSPEC, AF_INET +#endif #include "opentelemetry/ext/http/server/socket_tools.h" @@ -137,12 +140,4 @@ TEST(SocketAddrTest, RejectsPortOverflow) ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:4294967377")); } -// inet_pton() requires a canonical four-octet address, so it rejects the shorthand (127.1) and -// leading-zero (01.02.03.004) forms that the legacy inet_aton()/inet_addr() resolvers accepted. -TEST(SocketAddrTest, RejectsNonDottedQuadHost) -{ - ExpectInvalid(SocketTools::SocketAddr("127.1:80")); - ExpectInvalid(SocketTools::SocketAddr("01.02.03.004:80")); -} - } // namespace From 12a7e3ec8fdcf5e94e43839ba57cdd4194fc5425 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:34:29 +0800 Subject: [PATCH 09/11] Re-add portable shorthand-host rejection test and document the parser contract Re-add RejectsShorthandHost: inet_pton rejects the shorthand form (127.1) on every platform (verified on glibc and macOS), unlike the legacy inet_aton/inet_addr resolvers. The earlier removal dropped this portable case along with the platform-dependent leading-zero one; only leading-zero (01.02.03.004) is divergent (glibc rejects, macOS accepts). Document the SocketAddr(char const*) public contract in the header: host grammar follows the platform inet_pton, the port range and omitted-port-is-0 rule, and the AF_UNSPEC / port()==-1 invalid state. --- ext/include/opentelemetry/ext/http/server/socket_tools.h | 4 ++++ ext/test/http/socket_tools_test.cc | 9 +++++++++ 2 files changed, 13 insertions(+) diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index 162a0c7456..9e70653383 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -182,6 +182,10 @@ struct SocketAddr inet4.sin_addr.s_addr = htonl(addr); } + /// Parses an IPv4 address in "host" or "host:port" form. Host parsing follows the platform's + /// inet_pton(AF_INET), which requires four decimal components; a port, when present, must be + /// decimal digits only in the range 0..65535, and an omitted port is represented as 0. Invalid + /// input leaves the address at AF_UNSPEC, for which port() returns -1. SocketAddr(char const *addr) { // One parser for every platform: inet_pton (Winsock provides it since Vista) plus a strict diff --git a/ext/test/http/socket_tools_test.cc b/ext/test/http/socket_tools_test.cc index 08f7e2235d..a3bfa29bc2 100644 --- a/ext/test/http/socket_tools_test.cc +++ b/ext/test/http/socket_tools_test.cc @@ -140,4 +140,13 @@ TEST(SocketAddrTest, RejectsPortOverflow) ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:4294967377")); } +// inet_pton() requires four decimal components, so it rejects the shorthand form ("127.1") that the +// legacy inet_aton()/inet_addr() resolvers accepted. This holds on every platform (verified on +// glibc and macOS). Leading-zero components ("01.02.03.004") are deliberately not asserted: glibc +// rejects them but macOS inet_pton accepts them, so that outcome is platform-dependent. +TEST(SocketAddrTest, RejectsShorthandHost) +{ + ExpectInvalid(SocketTools::SocketAddr("127.1:80")); +} + } // namespace From 07f61bb264bfb7e1d18897b26f9a3dc1620fb367 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:45:52 +0800 Subject: [PATCH 10/11] Rewrite a test comment to describe the invariant, not code history Per maintainer guidance, code comments describe the code, not a previous revision. State the null-termination invariant and failure mode rather than what the buffer copy used to do. --- ext/test/http/socket_tools_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/test/http/socket_tools_test.cc b/ext/test/http/socket_tools_test.cc index a3bfa29bc2..d9f3f752aa 100644 --- a/ext/test/http/socket_tools_test.cc +++ b/ext/test/http/socket_tools_test.cc @@ -29,8 +29,8 @@ TEST(SocketAddrTest, ParsesHostAndPort) EXPECT_EQ(addr.toString(), "127.0.0.1:8800"); } -// The host part used to be copied into an uninitialized buffer that was terminated at a fixed -// index, so inet_pton read indeterminate bytes for any host shorter than 15 characters. +// A host shorter than the buffer must be null-terminated at its actual length; terminating at a +// fixed index would leave inet_pton reading indeterminate bytes for any host under 15 characters. TEST(SocketAddrTest, ParsesHostShorterThanTheBuffer) { SocketTools::SocketAddr addr("1.2.3.4:80"); From 757a3c414a1937b735855b6ff62c186d98e9cb39 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 26 Jul 2026 02:30:30 +0800 Subject: [PATCH 11/11] Clarify the overlong-host test and point the alignment note at #4307 Rename the overlong-input test to RejectsExtremelyOverlongHost with a comment describing the length bound it protects, rather than the historical Windows buffer bug. Point the two string-parser alignment notes at the dedicated #4307 (the integer constructor's reinterpret_cast and port truncation) instead of the broader #4287 tracker. --- ext/include/opentelemetry/ext/http/server/socket_tools.h | 4 ++-- ext/test/http/socket_tools_test.cc | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index 9e70653383..5b0a1ab487 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -192,7 +192,7 @@ struct SocketAddr // decimal port. This avoids WSAStringToAddress, whose grammar and default-component filling // differ from the POSIX path. Parse into a local sockaddr_in and commit with memcpy only on // success, which keeps m_data at AF_UNSPEC on failure and avoids accessing the sockaddr - // storage through a sockaddr_in glvalue (an alignment/type-access issue tracked in #4287). + // storage through a sockaddr_in glvalue (an alignment/type-access issue tracked in #4307). if (addr == nullptr) { LOG_WARN("SocketAddr: cannot parse a null address"); @@ -273,7 +273,7 @@ struct SocketAddr { case AF_INET: { // Copy out rather than binding a sockaddr_in glvalue to sockaddr storage, which is an - // alignment/type-access issue (see the constructor and #4287). + // alignment/type-access issue (see the constructor and #4307). sockaddr_in inet4{}; memcpy(&inet4, &m_data, sizeof(inet4)); return ntohs(inet4.sin_port); diff --git a/ext/test/http/socket_tools_test.cc b/ext/test/http/socket_tools_test.cc index d9f3f752aa..34f9dac51b 100644 --- a/ext/test/http/socket_tools_test.cc +++ b/ext/test/http/socket_tools_test.cc @@ -97,10 +97,9 @@ TEST(SocketAddrTest, RejectsInvalidHost) ExpectInvalid(SocketTools::SocketAddr("garbage")); } -// On Windows the copy loop was bounded by sizeof(buf), a byte count, rather than by the element -// count of a WCHAR array, so an address longer than 200 characters wrote past the end of the -// buffer. This must be handled rather than crash, and reported as a rejection. -TEST(SocketAddrTest, HandlesOverlongInput) +// A host far longer than the 15-character dotted-quad maximum must be rejected by the length +// bound before it can drive an out-of-bounds access on the fixed host buffer. +TEST(SocketAddrTest, RejectsExtremelyOverlongHost) { const std::string overlong(512, '9'); SocketTools::SocketAddr addr(overlong.c_str());