Skip to content

Ensure SocketOptionImpl.value_.data() is pointer-size-aligned.#38707

Merged
RyanTheOptimist merged 1 commit intoenvoyproxy:mainfrom
wu-bin:align
Mar 12, 2025
Merged

Ensure SocketOptionImpl.value_.data() is pointer-size-aligned.#38707
RyanTheOptimist merged 1 commit intoenvoyproxy:mainfrom
wu-bin:align

Conversation

@wu-bin
Copy link
Copy Markdown
Contributor

@wu-bin wu-bin commented Mar 11, 2025

This should fix #7968 for good.

Commit Message: Ensure SocketOptionImpl.value_.data() is pointer-size-aligned.
Additional Description:
Risk Level: None
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:

@wu-bin
Use a custom allocator to ensure SocketOptionImpl.value_.data() is
7019a75 
pointer-size-aligned, which is expected by the ASSERT in
SocketOptionImpl's constructor.

Signed-off-by: Bin Wu <wub@google.com>
@wu-bin
Copy link
Copy Markdown
Contributor Author

wu-bin commented Mar 11, 2025

/assign @RyanTheOptimist

@wu-bin
Copy link
Copy Markdown
Contributor Author

wu-bin commented Mar 12, 2025

/retest

RyanTheOptimist
RyanTheOptimist approved these changes Mar 12, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is such a great CL. Thanks for doing this!

source/common/network/socket_option_impl.h
const std::vector<uint8_t> value_;
// The vector's data() is used by the setsockopt syscall, which needs to be int-size-aligned on
// some platforms, the AlignedAllocator here makes it pointer-size-aligned, which satisfies the
// requirement, although it can be slightly over-aligned.
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent comment!

@RyanTheOptimist RyanTheOptimist merged commit 9d4bd29 into envoyproxy:main Mar 12, 2025
25 checks passed
@birenroy
Copy link
Copy Markdown
Contributor

birenroy commented Mar 13, 2025

FYI, it looks like maybe this caused a build issue in Envoy Mobile. @fredyw @abeyad

I'm not sure what the right resolution should be.

@abeyad
Copy link
Copy Markdown
Contributor

abeyad commented Mar 13, 2025 •
edited
Loading

@wu-bin which platforms were the ASAN tests failing on? Our Android NDK version (same version YT is using) doesn't support std::aligned_alloc. Fredy mentioned that std::aligned_alloc was introduced in NDK version 28. Can this code be ifdef guarded by:

#ifdef __ANDROID_API__
#if __ANDROID_API__ >= 28

// std::aligned_alloc usage goes here

#endif // __ANDROID_API__ >= 28
#endif // ifdef __ANDROID_API__

Otherwise, Envoy Mobile won't compile unfortunately on Android..

@abeyad
Copy link
Copy Markdown
Contributor

abeyad commented Mar 13, 2025

FYI, this is the CI failure: https://github.com/envoyproxy/envoy/actions/runs/13818727995/job/38658598098

abeyad added a commit to abeyad/envoy that referenced this pull request Mar 13, 2025
@abeyad
ci: Add trigger for Android and iOS CI jobs on source/common/* changes
fcc9b2c 
There have been several instances that changes, particularily to
source/* directories, have introduced C++ features not supported by the
Android NDK or iOS XCode version for Envoy Mobile.  Without triggering
the mobile Android and iOS jobs on those changes, we don't catch them
until after the fact.

The latest example is the std::aligned_alloc PR:
envoyproxy#38707.

Which introduced build errors on Android that CI didn't catch at the
time of the PR and only in a subsequent PR that triggered mobile CI:
https://github.com/envoyproxy/envoy/actions/runs/13818727995/job/38658598098.

This change causes any changes to `source/common/` to trigger Android
and iOS CI.  This is a start in allowing us to catch such changes in the
PR making the changes.

Signed-off-by: Ali Beyad <abeyad@google.com>
@abeyad abeyad mentioned this pull request Mar 13, 2025
Merged
abeyad added a commit to abeyad/envoy that referenced this pull request Mar 13, 2025
@abeyad
common: Disable pointer-size-alignment on SocketOptionImpl
0d08321 
envoyproxy#38707 enabled a new aligned
allocator using `std::aligned_alloc`. However, `std::aligned_alloc` is
not available on the Android NDK we are using for Envoy Mobile, thereby
causing Envoy Mobile Android CI to fail.

This PR temporarily disables the aligned allocator on Android until a
more permanent solution is implemented.

Signed-off-by: Ali Beyad <abeyad@google.com>
@abeyad abeyad mentioned this pull request Mar 13, 2025
Closed
abeyad added a commit to abeyad/envoy that referenced this pull request Mar 13, 2025
@abeyad
common: Disable pointer-size-alignment on SocketOptionImpl
07cfbdb 
envoyproxy#38707 enabled a new aligned
allocator using `std::aligned_alloc`. However, `std::aligned_alloc` is
not available on the Android NDK we are using for Envoy Mobile, thereby
causing Envoy Mobile Android CI to fail.

This PR temporarily disables the aligned allocator on Android until a
more permanent solution is implemented.

Signed-off-by: Ali Beyad <abeyad@google.com>
abeyad added a commit to abeyad/envoy that referenced this pull request Mar 13, 2025
@abeyad
common: Disable pointer-size-alignment on SocketOptionImpl
a204053 
envoyproxy#38707 enabled a new aligned
allocator using `std::aligned_alloc`. However, `std::aligned_alloc` is
not available on the Android NDK we are using for Envoy Mobile, thereby
causing Envoy Mobile Android CI to fail.

This PR temporarily disables the aligned allocator on Android until a
more permanent solution is implemented.

Signed-off-by: Ali Beyad <abeyad@google.com>
@wu-bin
Copy link
Copy Markdown
Contributor Author

wu-bin commented Mar 13, 2025 •
edited
Loading

Thanks everyone for the comments! I'm able to reproduce it, I'll send a fix to use posix_memalign on the older Android versions.

Edit: I just sent #38735 to use posix_memalign.

phlax pushed a commit that referenced this pull request Mar 14, 2025
@abeyad
ci: Add trigger for Android and iOS CI jobs on source/common changes (#…
7cb808b 
…38733)

There have been several instances that changes, particularily to
source/* directories, have introduced C++ features not supported by the
Android NDK or iOS XCode version for Envoy Mobile. Without triggering
the mobile Android and iOS jobs on those changes, we don't catch them
until after the fact.

The latest example is the std::aligned_alloc PR:
#38707.

Which introduced build errors on Android that CI didn't catch at the
time of the PR and only in a subsequent PR that triggered mobile CI:
https://github.com/envoyproxy/envoy/actions/runs/13818727995/job/38658598098.

This change causes any changes to `source/common/` to trigger Android
and iOS CI. This is a start in allowing us to catch such changes in the
PR making the changes.

Signed-off-by: Ali Beyad <abeyad@google.com>
jewertow pushed a commit to jewertow/envoy that referenced this pull request Apr 2, 2025
@wu-bin @jewertow
Ensure SocketOptionImpl.value_.data() is pointer-size-aligned. (envoy…
422cfa2 
…proxy#38707)

This should fix envoyproxy#7968 for
good.

Commit Message: Ensure SocketOptionImpl.value_.data() is
pointer-size-aligned.
Additional Description:
Risk Level: None
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Bin Wu <wub@google.com>
jewertow pushed a commit to jewertow/envoy that referenced this pull request Apr 2, 2025
@abeyad @jewertow
ci: Add trigger for Android and iOS CI jobs on source/common changes (e…
25d13cd 
…nvoyproxy#38733)

There have been several instances that changes, particularily to
source/* directories, have introduced C++ features not supported by the
Android NDK or iOS XCode version for Envoy Mobile. Without triggering
the mobile Android and iOS jobs on those changes, we don't catch them
until after the fact.

The latest example is the std::aligned_alloc PR:
envoyproxy#38707.

Which introduced build errors on Android that CI didn't catch at the
time of the PR and only in a subsequent PR that triggered mobile CI:
https://github.com/envoyproxy/envoy/actions/runs/13818727995/job/38658598098.

This change causes any changes to `source/common/` to trigger Android
and iOS CI. This is a start in allowing us to catch such changes in the
PR making the changes.

Signed-off-by: Ali Beyad <abeyad@google.com>
agrawroh pushed a commit to agrawroh/envoy that referenced this pull request Apr 9, 2025
@wu-bin @agrawroh
Ensure SocketOptionImpl.value_.data() is pointer-size-aligned. (envoy…
1968205 
…proxy#38707)

This should fix envoyproxy#7968 for
good.

Commit Message: Ensure SocketOptionImpl.value_.data() is
pointer-size-aligned.
Additional Description:
Risk Level: None
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Bin Wu <wub@google.com>
agrawroh pushed a commit to agrawroh/envoy that referenced this pull request Apr 9, 2025
@abeyad @agrawroh
ci: Add trigger for Android and iOS CI jobs on source/common changes (e…
91505dd 
…nvoyproxy#38733)

There have been several instances that changes, particularily to
source/* directories, have introduced C++ features not supported by the
Android NDK or iOS XCode version for Envoy Mobile. Without triggering
the mobile Android and iOS jobs on those changes, we don't catch them
until after the fact.

The latest example is the std::aligned_alloc PR:
envoyproxy#38707.

Which introduced build errors on Android that CI didn't catch at the
time of the PR and only in a subsequent PR that triggered mobile CI:
https://github.com/envoyproxy/envoy/actions/runs/13818727995/job/38658598098.

This change causes any changes to `source/common/` to trigger Android
and iOS CI. This is a start in allowing us to catch such changes in the
PR making the changes.

Signed-off-by: Ali Beyad <abeyad@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RyanTheOptimist RyanTheOptimist RyanTheOptimist approved these changes

Assignees

@RyanTheOptimist RyanTheOptimist

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AddressSanitizer: misaligned access in setsockopt()

4 participants

@wu-bin @birenroy @abeyad @RyanTheOptimist