Conversation
Factorized-table tuples are packed without alignment padding, so an
aggregate state column can start at an odd offset (e.g. after a small
group-by key). HashAggregateScan reinterpreted those bytes as
AggregateState* and HashAggregateScan/getMoveAggResultToVectorFuncs
invoked virtuals on it:
- map_aggregate.cpp:106: aggregateState->writeToVector() loads the vptr
from a misaligned address ("member access within misaligned address
... for type 'struct AggregateState', which requires 8 byte
alignment"), and the WithNull path has the same problem via
constCast<AggregateStateWithNull>().isNull.
- hash_aggregate_scan.cpp: offset += aggState->getStateSize() is a
virtual call on the same misaligned pointer.
Fix, following the memcpy precedent of fafc362/2092c4284:
- Capture each aggregate's state size at plan time and copy the state
bytes into an aligned thread-local buffer before any virtual
dispatch in the move-to-vector funcs.
- Advance the scan offset using the table schema's state column sizes
instead of the virtual getStateSize().
Nightly run: https://github.com/LadybugDB/ladybug/actions/runs/35438024832/job/105947162035
…438024832 Same bug class as the scan-path fix: packed/unaligned storage read through typed pointers. - function/aggregate_function.h: route updateAll/updatePos/combine/ finalize through an aligned thread-local staging buffer when the state pointer fails a max_align_t check (covers SumState, AvgState, MinMaxState, CollectState, HistogramState, PercentileCont/DiscState update/combine/finalize paths at a single choke point; aligned states go straight through). - common/in_mem_overflow_buffer.cpp: keep bump allocations 8-byte aligned (COLLECT/HISTOGRAM linked-list elements store pointers). - processor/result/base_hash_table.cpp: memcpy list_t out of packed tuples before touching members (both list compare entry points). - function/find_function.cpp: memcpy word-sized needle/haystack comparisons (haystack+offset is inherently misaligned). - parquet interval_column_reader.cpp: memcpy interval fields out of the unaligned plain-data buffer. Still open (different bug class, needs semantic decisions): integer overflow/negation/shift reports (int128_t.cpp, hash_aggregate.h shift-64, std::abs, compression.cpp negation).
Contributor
Author
|
Addressed via #1014 |
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
1 participant
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.
Fixes the ASan+UBSan nightly failure (
map_aggregate.cpp:106: member access within misaligned address ... for type 'struct AggregateState').Root cause: factorized-table tuples are packed without alignment padding, so an aggregate state column can start at an odd offset (e.g. after a small group-by key).
HashAggregateScanreinterpreted those bytes asAggregateState*and invoked virtuals on the misaligned pointer:writeAggResultWithoutNullToVector(aggregateState->writeToVector()) loads the vptr from a misaligned address; the WithNull path has the same problem viaconstCast<AggregateStateWithNull>().isNull.hash_aggregate_scan.cpp:offset += aggState->getStateSize()is a virtual call on the same misaligned pointer.Fix (follows the memcpy precedent of fafc362/2092c4284):
map_aggregate.cpp: capture each aggregate's state size at plan time and copy the state bytes into an aligned thread-local buffer before any virtual dispatch in the move-to-vector funcs. The copy carries the vptr bit pattern, so the dynamic type is preserved; thread-local keeps it race-free across scan threads.hash_aggregate_scan.cpp: advance the scan offset using the table schema's state column sizes (sized withAggregateFunction::getAggregateStateSize()at plan time) instead of the virtualgetStateSize().Nightly run: https://github.com/LadybugDB/ladybug/actions/runs/35438024832/job/105947162035
Note: that log contains further reports of the same class of bug in other aggregate states (
sum.h,min_max.h,collect.cpp,histogram.cpp,percentile_*.cppupdate/combine paths,base_hash_table.cpplist_t). Left for follow-up PRs to keep this one focused.