[BUG] Make SocketAddr string parsing safe and reject malformed addresses

CHANGELOG.md

@@ -22,6 +22,9 @@ Increment the:
 * [CODE HEALTH] Move remaining API test helpers into anonymous namespaces
   [#4301](https://github.com/open-telemetry/opentelemetry-cpp/pull/4301)
 
+* [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
   [#4286](https://github.com/open-telemetry/opentelemetry-cpp/pull/4286)
 

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",
 )

ext/CMakeLists.txt

@@ -10,6 +10,14 @@ 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

ext/include/opentelemetry/ext/http/server/socket_tools.h

@@ -22,6 +22,7 @@
 // #  include <windows.h>
 
 #  include <winsock2.h>
+#  include <ws2tcpip.h>  // inet_pton
 
 // TODO: consider NOMINMAX
 #  undef min
@@ -181,46 +182,85 @@ 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)
   {
-#ifdef _WIN32
-    INT addrlen = sizeof(m_data);
-    WCHAR buf[200];
-    for (int i = 0; i < sizeof(buf) && addr[i]; i++)
+    // 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 #4307).
+    if (addr == nullptr)
     {
-      buf[i] = addr[i];
+      LOG_WARN("SocketAddr: cannot parse a null address");
+      return;  // m_data is already AF_UNSPEC, so port() reports -1.
     }
-    buf[199] = L'\0';
-    ::WSAStringToAddressW(buf, AF_INET, nullptr, &m_data, &addrlen);
-#else
-    sockaddr_in &inet4 = reinterpret_cast<sockaddr_in &>(m_data);
-    inet4.sin_family   = AF_INET;
-    char const *colon  = strchr(addr, ':');
-    if (colon)
+
+    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;
+
+    // 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"). No dotted-quad
+    // exceeds 15 characters.
+    bool ok = (hostLength >= 1 && hostLength <= 15);
+    if (ok)
     {
-      char *portEnd     = nullptr;
-      errno             = 0;
-      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)
+      char host[16];
+      memcpy(host, addr, static_cast<size_t>(hostLength));
+      host[hostLength] = '\0';
+      ok               = (::inet_pton(AF_INET, host, &parsed.sin_addr) == 1);
+    }
+
+    // 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 const *p     = colon + 1;
+      unsigned int port = 0;
+      if (*p == '\0')
       {
-        inet4.sin_port = htons(static_cast<uint16_t>(parsed));
+        ok = false;  // empty port, e.g. "127.0.0.1:"
       }
-      else
+      while (ok && *p != '\0')
+      {
+        if (*p < '0' || *p > '9')
+        {
+          ok = false;
+          break;
+        }
+        unsigned int const digit = static_cast<unsigned int>(*p - '0');
+        if (port > (65535u - digit) / 10u)
+        {
+          ok = false;  // would exceed 65535
+          break;
+        }
+        port = port * 10u + digit;
+        ++p;
+      }
+      if (ok)
       {
-        inet4.sin_port = 0;
+        parsed.sin_port = htons(static_cast<uint16_t>(port));
       }
-      char buf[16];
-      memcpy(buf, addr, (std::min<ptrdiff_t>)(15, colon - addr));
-      buf[15] = '\0';
-      ::inet_pton(AF_INET, buf, &inet4.sin_addr);
+    }
+
+    if (ok)
+    {
+      memcpy(&m_data, &parsed, sizeof(parsed));
     }
     else
     {
-      inet4.sin_port = 0;
-      ::inet_pton(AF_INET, addr, &inet4.sin_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; }
@@ -232,7 +272,10 @@ struct SocketAddr
     switch (m_data.sa_family)
     {
       case AF_INET: {
-        sockaddr_in const &inet4 = reinterpret_cast<sockaddr_in const &>(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 #4307).
+        sockaddr_in inet4{};
+        memcpy(&inet4, &m_data, sizeof(inet4));
         return ntohs(inet4.sin_port);
       }
 
@@ -248,8 +291,9 @@ struct SocketAddr
     switch (m_data.sa_family)
     {
       case AF_INET: {
-        sockaddr_in const &inet4 = reinterpret_cast<sockaddr_in const &>(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);
@@ -263,6 +307,18 @@ struct SocketAddr
   }
 };
 
+// The parser memcpys a sockaddr_in into m_data, and the socket syscalls pass sizeof(SocketAddr)
+// 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),
+              "SocketAddr must add no storage beyond its sockaddr, since syscalls use its size");
+
 /// <summary>
 /// Encapsulation of a socket (non-exclusive ownership)
 /// </summary>

ext/test/http/BUILD

@@ -3,6 +3,20 @@
 
 load("@rules_cc//cc:cc_test.bzl", "cc_test")
 
+cc_test(
+    name = "socket_tools_test",
+    srcs = [
+        "socket_tools_test.cc",
+    ],
+    # 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",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 cc_test(
     name = "curl_http_test",
     srcs = [

ext/test/http/CMakeLists.txt

@@ -14,6 +14,18 @@ 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})
+# 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.
+  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}

ext/test/http/socket_tools_test.cc

@@ -0,0 +1,151 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+#include <gtest/gtest.h>
+#include <string>
+#ifndef _WIN32
+#  include <sys/socket.h>  // for sockaddr, AF_UNSPEC, AF_INET
+#endif
+
+#include "opentelemetry/ext/http/server/socket_tools.h"
+
+namespace
+{
+
+// 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");
+}
+
+// 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");
+  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");
+}
+
+// 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.m_data.sa_family, AF_INET);
+  EXPECT_EQ(addr.port(), 0);
+  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");
+  ExpectInvalid(addr);
+}
+
+// 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");
+  ExpectInvalid(addr);
+}
+
+TEST(SocketAddrTest, RejectsTrailingGarbageInPort)
+{
+  ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:80junk"));
+  ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:80:90"));
+}
+
+TEST(SocketAddrTest, RejectsInvalidHost)
+{
+  ExpectInvalid(SocketTools::SocketAddr("999.999.999.999:4318"));
+  ExpectInvalid(SocketTools::SocketAddr("garbage"));
+}
+
+// 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());
+  ExpectInvalid(addr);
+}
+
+TEST(SocketAddrTest, HandlesEmptyInput)
+{
+  SocketTools::SocketAddr addr("");
+  ExpectInvalid(addr);
+}
+
+TEST(SocketAddrTest, RejectsNullInput)
+{
+  SocketTools::SocketAddr addr(nullptr);
+  ExpectInvalid(addr);
+}
+
+TEST(SocketAddrTest, RejectsEmptyHostOrPort)
+{
+  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)
+{
+  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)
+{
+  ExpectInvalid(SocketTools::SocketAddr("127.0.0.1:65536"));
+  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