Fix UB: Remove flexible array member from Operation class#1486
Closed
taoting1234 wants to merge 2 commits into
Closed
Fix UB: Remove flexible array member from Operation class#1486taoting1234 wants to merge 2 commits into
taoting1234 wants to merge 2 commits into
Conversation
Replace char _buf[0] (flexible array member, UB in C++) with explicit buffer pointer management to avoid undefined behavior. ## Problem with Clang When compiled with Clang under -O2 optimization, the flexible array member causes the compiler to optimize away the default constructor of Operation. This leads to uninitialized members (e.g., StoredURL proxy_url), causing SEGV crashes when the destructor attempts to free uninitialized pointers. Example crash: ==ERROR: AddressSanitizer: SEGV on unknown address #0 URL::~URL() at url.h:58:9 #1 StoredURL::~StoredURL() at url.h:113:5 alibaba#2 Operation::~Operation() at client.h:142:30 The issue occurs because: 1. Flexible array member (char _buf[0]) is not standard C++ 2. Clang treats the class as having trivial initialization 3. Under -O2, the constructor call is optimized away 4. Stack-allocated OperationOnStack objects have uninitialized members ## Changes - Replace char _buf[0] with char* _buf pointing to external buffer - Add internal constructors accepting buffer pointer parameter - Heap-allocated Operation (via create()) uses buffer after object (this + 1) - Stack-allocated OperationOnStack passes its _buf array to base constructor This eliminates two sources of UB: 1. Flexible array member (char _buf[0]) - not standard C++ 2. Derived class overriding base class member (char _buf[BufferSize]) The fix ensures correct behavior regardless of compiler optimizations and eliminates potential issues with -O2 or different compiler versions. Verified: DadiCacheTest passes with ASAN enabled, no SEGV crashes.
Add test to verify OperationOnStack correctly initializes all members, particularly proxy_url. This test catches the UB caused by flexible array member where Clang -O2 optimizes away the constructor, leaving proxy_url.m_url uninitialized (garbage pointer), which causes SEGV when the destructor attempts to free it. The test: - Creates OperationOnStack with various buffer sizes - Verifies proxy_url.empty() returns true (m_url == nullptr) - Lets objects go out of scope to trigger destructor - With UB, destructor would SEGV on free(garbage) Note: This issue only manifests with Clang under -O2 optimization. GCC and Clang -O0 may not trigger the problematic optimization, but the test ensures correct behavior across all compilers.
Collaborator
|
As flexible array is a standard feature of C, it is highly unlikely that a decent compiler breaks it with optimization on. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace char _buf[0] (flexible array member, UB in C++) with explicit buffer pointer management to avoid undefined behavior.
Problem with Clang
When compiled with Clang under -O2 optimization, the flexible array member causes the compiler to optimize away the default constructor of Operation. This leads to uninitialized members (e.g., StoredURL proxy_url), causing SEGV crashes when the destructor attempts to free uninitialized pointers.
Example crash:
==ERROR: AddressSanitizer: SEGV on unknown address
#0 URL::~URL() at url.h:58:9
#1 StoredURL::~StoredURL() at url.h:113:5
#2 Operation::~Operation() at client.h:142:30
The issue occurs because:
Changes
This eliminates two sources of UB:
The fix ensures correct behavior regardless of compiler optimizations and eliminates potential issues with -O2 or different compiler versions.
Verified: DadiCacheTest passes with ASAN enabled, no SEGV crashes.