[JonSnow1807]

Fixes #4307

Changes

SocketAddr was accessing a sockaddr_in through a cast that could produce a misaligned reference. UBSan reports this in the existing implementation.

The change makes SocketAddr store a sockaddr_storage instead, so the address has the alignment required by the supported socket address types.

The default constructor now initializes the family to AF_UNSPEC. I also added addr() and addr_len() accessors and updated the bind/connect call sites to use them with the correct IPv4 address length.

This follows the general approach @marcalff suggested in the earlier PR discussions.

Testing

I added a padded-structure regression test that fails against the original header even without sanitizers.

UBSan reports a misaligned sockaddr_in reference before the fix. The updated implementation passes the regression test, both normally and under UBSan.

All 21 socket tests pass in both configurations.

A CHANGELOG entry is included.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: JonSnow1807 / name: JonSnow1807 (9d23f09)

[JonSnow1807]

/easycla

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.60%. Comparing base (dc5bd6e) to head (2bbdd82).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...clude/opentelemetry/ext/http/server/socket_tools.h	91.67%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4725      +/-   ##
==========================================
+ Coverage   86.57%   86.60%   +0.03%     
==========================================
  Files         529      530       +1     
  Lines       20996    21010      +14     
==========================================
+ Hits        18176    18194      +18     
+ Misses       2820     2816       -4     

Files with missing lines	Coverage Δ
...clude/opentelemetry/ext/http/server/socket_tools.h	95.70% <91.67%> (+0.03%)	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[marcalff]

@JonSnow1807

Thanks for picking this up.

There is a new clang-tidy warning modernize-deprecated-headers, please check the logs in CI for details, and fix.

(edit)

Full warning is:

**modernize-deprecated-headers** (1 warnings)

| File | Line | Message |
|---|---|---|
| `opentelemetry-cpp/ext/test/http/socket_tools_test.cc` | 5 | inclusion of deprecated C++ header 'stddef.h'; consider using 'cstddef' instead |

[mateenali66]

checked this against marcalff's #4629 notes. it matches except the (u_long, uint16_t) constructor, where the note kept the cast (edited). outside the header and its test, the only SocketAddr users are the two in http_server.h, both go through the updated Socket methods, and nothing relied on the removed conversion operators. 21 of 21 socket tests pass at af1a2cc with Apple clang 21, plain and under UBSan, and with GCC 16. making addr_len() return the storage size fails bind() on macOS, and two of the new tests catch it.

one thing on ConstructsFromHostOrderAddressAndPort. ported onto the old header (sa_family, minus the addr() test), it fails with GCC 16 as described, but passes with Apple clang 21 at -O0 through -O3, because clang places padded so the member lands 4-byte aligned.

static_assert(alignof(decltype(SocketTools::SocketAddr::m_data)) >= alignof(sockaddr_in), "") fails on the old header with both compilers. could the test assert that in the next push?

[marcalff]

Thanks for the contribution.

This is a much better patch for this issue.

Please see some comments, and also the CI failure on clang-tidy.

I will approve and merge once comments are addressed, the patch looks good overall.

[JonSnow1807]

Thanks for the review. All four comments are addressed in 776f5b5, which is also rebased on current main:

• the memcpy sentence in the m_data comment is gone, the (u_long, uint16_t) constructor is back to writing through the sockaddr_in reference, the static_assert comment uses your wording, and BindsConnectsAndAcceptsOnLoopback is removed.
• clang-tidy: the test includes instead of <stddef.h>.
• following @mateenali66's suggestion, the test now has static_assert(alignof(decltype(SocketTools::SocketAddr::m_data)) >= alignof(sockaddr_in), ...), which fails on the old header regardless of compiler.

Locally: socket_tools_test 20/20 in a normal build and under UBSan (no runtime errors), url_parser_test 36/36, curl_http_test 39 pass with the usual 1 skip.

[lalitb]

Thanks.

[marcalff]

LGTM, thanks for the fix.