From 2a11c57a5713e5d1a4f26fe059b90ca705c517d9 Mon Sep 17 00:00:00 2001 From: Arun Sharma Date: Sat, 19 Sep 2026 22:34:28 -0700 Subject: [PATCH 1/2] Fix UBSan misaligned AggregateState access in hash aggregate scan 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().isNull. - hash_aggregate_scan.cpp: offset += aggState->getStateSize() is a virtual call on the same misaligned pointer. Fix, following the memcpy precedent of fafc36265/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 --- src/processor/map/map_aggregate.cpp | 42 +++++++++++++++---- .../aggregate/hash_aggregate_scan.cpp | 10 ++++- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/src/processor/map/map_aggregate.cpp b/src/processor/map/map_aggregate.cpp index 1e9deb9db4..fb863c7b97 100644 --- a/src/processor/map/map_aggregate.cpp +++ b/src/processor/map/map_aggregate.cpp @@ -1,4 +1,6 @@ #include +#include +#include #include "binder/expression/aggregate_function_expression.h" #include "binder/expression/literal_expression.h" @@ -91,29 +93,55 @@ static std::vector getAggFunctions(const expression_vector& a return aggregateFunctions; } +// Aggregate states are stored in packed factorized-table tuples without alignment padding, +// so a state pointer derived from a tuple may be misaligned for AggregateState (which +// requires 8-byte alignment because of its vptr). Virtual dispatch on such a pointer -- and +// member access inside the callee -- is UB, reported by UBSan as "member access within +// misaligned address". Copy the state bytes into an aligned thread-local buffer first +// (memcpy has no alignment requirement) and dispatch on the aligned copy. The copy carries +// the vptr bit pattern, so the dynamic type is preserved. Thread-local storage keeps the +// buffer reusable across calls without data races between scan threads. +static AggregateState* copyStateToAlignedBuffer(AggregateState* aggregateState, + uint32_t stateSize) { + thread_local std::vector alignedBuffer; + if (alignedBuffer.size() < stateSize) { + alignedBuffer.resize(stateSize); + } + memcpy(alignedBuffer.data(), aggregateState, stateSize); + return reinterpret_cast(alignedBuffer.data()); +} + static void writeAggResultWithNullToVector(ValueVector& vector, uint64_t pos, - AggregateState* aggregateState) { - auto isNull = aggregateState->constCast().isNull; + AggregateState* aggregateState, uint32_t stateSize) { + auto* alignedState = copyStateToAlignedBuffer(aggregateState, stateSize); + auto isNull = alignedState->constCast().isNull; vector.setNull(pos, isNull); if (!isNull) { - aggregateState->writeToVector(&vector, pos); + alignedState->writeToVector(&vector, pos); } } static void writeAggResultWithoutNullToVector(ValueVector& vector, uint64_t pos, - AggregateState* aggregateState) { + AggregateState* aggregateState, uint32_t stateSize) { vector.setNull(pos, false); - aggregateState->writeToVector(&vector, pos); + copyStateToAlignedBuffer(aggregateState, stateSize)->writeToVector(&vector, pos); } static std::vector getMoveAggResultToVectorFuncs( std::vector& aggregateFunctions) { std::vector moveAggResultToVectorFuncs; for (auto& aggregateFunction : aggregateFunctions) { + auto stateSize = static_cast(aggregateFunction.getAggregateStateSize()); if (aggregateFunction.needToHandleNulls) { - moveAggResultToVectorFuncs.push_back(writeAggResultWithoutNullToVector); + moveAggResultToVectorFuncs.push_back( + [stateSize](ValueVector& vector, uint64_t pos, AggregateState* aggregateState) { + writeAggResultWithoutNullToVector(vector, pos, aggregateState, stateSize); + }); } else { - moveAggResultToVectorFuncs.push_back(writeAggResultWithNullToVector); + moveAggResultToVectorFuncs.push_back( + [stateSize](ValueVector& vector, uint64_t pos, AggregateState* aggregateState) { + writeAggResultWithNullToVector(vector, pos, aggregateState, stateSize); + }); } } return moveAggResultToVectorFuncs; diff --git a/src/processor/operator/aggregate/hash_aggregate_scan.cpp b/src/processor/operator/aggregate/hash_aggregate_scan.cpp index e8ab4c2bd4..4e67087c75 100644 --- a/src/processor/operator/aggregate/hash_aggregate_scan.cpp +++ b/src/processor/operator/aggregate/hash_aggregate_scan.cpp @@ -24,14 +24,20 @@ bool HashAggregateScan::getNextTuplesInternal(ExecutionContext* /*context*/) { entries.resize(numRowsToScan); sharedState->scan(entries, groupByKeyVectors, startOffset, numRowsToScan, groupByKeyVectorsColIdxes); + // Aggregate states are packed without alignment padding, so `entry + offset` may be + // misaligned for AggregateState: never call virtuals (e.g. getStateSize()) on it here. + // State byte sizes come from the table schema instead (each state column was sized with + // AggregateFunction::getAggregateStateSize() at plan time); the move funcs copy each + // state to an aligned buffer before dispatching. + auto tableSchema = sharedState->getTableSchema(); for (auto pos = 0u; pos < numRowsToScan; ++pos) { auto entry = entries[pos]; - auto offset = sharedState->getTableSchema()->getColOffset(groupByKeyVectors.size()); + auto offset = tableSchema->getColOffset(groupByKeyVectors.size()); for (auto i = 0u; i < aggregateVectors.size(); i++) { auto vector = aggregateVectors[i]; auto aggState = reinterpret_cast(entry + offset); scanInfo.moveAggResultToVectorFuncs[i](*vector, pos, aggState); - offset += aggState->getStateSize(); + offset += tableSchema->getColumn(groupByKeyVectors.size() + i)->getNumBytes(); } } metrics->numOutputTuple.increase(numRowsToScan); From d0c12770f1edc01315a0d930b7496ac8d7280e03 Mon Sep 17 00:00:00 2001 From: Arun Sharma Date: Sat, 19 Sep 2026 22:50:38 -0700 Subject: [PATCH 2/2] Fix remaining UBSan misaligned accesses from nightly sanitizer run 35438024832 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). --- src/common/in_mem_overflow_buffer.cpp | 8 +++ src/function/find_function.cpp | 9 ++- src/include/function/aggregate_function.h | 70 +++++++++++++++++-- .../reader/parquet/interval_column_reader.cpp | 16 +++-- src/processor/result/base_hash_table.cpp | 17 ++++- 5 files changed, 107 insertions(+), 13 deletions(-) diff --git a/src/common/in_mem_overflow_buffer.cpp b/src/common/in_mem_overflow_buffer.cpp index b118c77787..be42c7eb7f 100644 --- a/src/common/in_mem_overflow_buffer.cpp +++ b/src/common/in_mem_overflow_buffer.cpp @@ -23,6 +23,14 @@ uint8_t* BufferBlock::data() const { } uint8_t* InMemOverflowBuffer::allocateSpace(uint64_t size) { + if (!blocks.empty()) { + // Keep every allocation 8-byte aligned: consumers store pointers and uint64s in + // overflow memory (e.g. COLLECT/HISTOGRAM linked-list elements), and an unaligned + // bump offset would make those accesses UB (UBSan: "store to misaligned address"). + static constexpr uint64_t OVERFLOW_ALIGNMENT = 8; + currentBlock()->currentOffset = + (currentBlock()->currentOffset + OVERFLOW_ALIGNMENT - 1) & ~(OVERFLOW_ALIGNMENT - 1); + } if (requireNewBlock(size)) { if (!blocks.empty() && currentBlock()->currentOffset == 0) { blocks.pop_back(); diff --git a/src/function/find_function.cpp b/src/function/find_function.cpp index 866eaf106d..dbd8f90a3b 100644 --- a/src/function/find_function.cpp +++ b/src/function/find_function.cpp @@ -47,9 +47,14 @@ int64_t Find::alignedNeedleSizeFind(const uint8_t* haystack, uint32_t haystackLe if (sizeof(UNSIGNED) > haystackLen) { return -1; } - auto needleVal = *((UNSIGNED*)needle); + // haystack + offset is inherently misaligned for most offsets (and needle itself comes + // from an unaligned string buffer), so compare through memcpy'd locals instead of + // dereferencing possibly-misaligned pointers (UBSan: "load of misaligned address"). + UNSIGNED needleVal; + memcpy(&needleVal, needle, sizeof(UNSIGNED)); for (auto offset = 0u; offset <= haystackLen - sizeof(UNSIGNED); offset++) { - auto haystackVal = *((UNSIGNED*)(haystack + offset)); + UNSIGNED haystackVal; + memcpy(&haystackVal, haystack + offset, sizeof(UNSIGNED)); if (needleVal == haystackVal) { return firstMatchCharOffset + offset; } diff --git a/src/include/function/aggregate_function.h b/src/include/function/aggregate_function.h index af7791504e..4fb617f80e 100644 --- a/src/include/function/aggregate_function.h +++ b/src/include/function/aggregate_function.h @@ -1,7 +1,11 @@ #pragma once +#include +#include +#include #include #include +#include #include "common/in_mem_overflow_buffer.h" #include "common/vector/value_vector.h" @@ -34,6 +38,37 @@ using aggr_combine_function_t = std::function; using aggr_finalize_function_t = std::function; +namespace detail { +// Aggregate states stored in packed factorized-table tuples may be misaligned for their +// concrete state type (tuples have no alignment padding). The per-function update/combine/ +// finalize implementations reinterpret_cast the raw bytes and touch members, which is UB on +// a misaligned pointer (UBSan: "member access within misaligned address"). Route every call +// through an aligned staging buffer: states that already satisfy the strictest fundamental +// alignment go straight through (the common case, e.g. heap-allocated simple-aggregate +// states); the rest are copied in with memcpy, executed on the aligned copy, and copied +// back. Two slots are provided so combine() can stage both inputs. See also +// copyStateToAlignedBuffer in processor/map/map_aggregate.cpp for the scan path. +inline constexpr size_t AGG_STATE_ALIGNMENT = alignof(std::max_align_t); + +inline bool isAggregateStateAligned(const uint8_t* state) { + return reinterpret_cast(state) % AGG_STATE_ALIGNMENT == 0; +} + +inline std::vector& stagedAggregateStateBuffer(int slot) { + thread_local std::vector buffers[2]; + return buffers[slot]; +} + +inline uint8_t* stageAggregateState(const uint8_t* state, uint32_t size, int slot) { + auto& buffer = stagedAggregateStateBuffer(slot); + if (buffer.size() < size) { + buffer.resize(size); + } + memcpy(buffer.data(), state, size); + return buffer.data(); +} +} // namespace detail + struct AggregateFunction final : public ScalarOrAggregateFunction { bool isDistinct; bool needToHandleNulls = false; @@ -75,20 +110,47 @@ struct AggregateFunction final : public ScalarOrAggregateFunction { void updateAllState(uint8_t* state, common::ValueVector* input, uint64_t multiplicity, common::InMemOverflowBuffer* overflowBuffer) const { - return updateAllFunc(state, input, multiplicity, overflowBuffer); + if (detail::isAggregateStateAligned(state)) { + return updateAllFunc(state, input, multiplicity, overflowBuffer); + } + auto size = static_cast(getAggregateStateSize()); + auto* staged = detail::stageAggregateState(state, size, 0); + updateAllFunc(staged, input, multiplicity, overflowBuffer); + memcpy(state, staged, size); } void updatePosState(uint8_t* state, common::ValueVector* input, uint64_t multiplicity, uint32_t pos, common::InMemOverflowBuffer* overflowBuffer) const { - return updatePosFunc(state, input, multiplicity, pos, overflowBuffer); + if (detail::isAggregateStateAligned(state)) { + return updatePosFunc(state, input, multiplicity, pos, overflowBuffer); + } + auto size = static_cast(getAggregateStateSize()); + auto* staged = detail::stageAggregateState(state, size, 0); + updatePosFunc(staged, input, multiplicity, pos, overflowBuffer); + memcpy(state, staged, size); } void combineState(uint8_t* state, uint8_t* otherState, common::InMemOverflowBuffer* overflowBuffer) const { - return combineFunc(state, otherState, overflowBuffer); + if (detail::isAggregateStateAligned(state) && detail::isAggregateStateAligned(otherState)) { + return combineFunc(state, otherState, overflowBuffer); + } + auto size = static_cast(getAggregateStateSize()); + auto* staged = detail::stageAggregateState(state, size, 0); + auto* stagedOther = detail::stageAggregateState(otherState, size, 1); + combineFunc(staged, stagedOther, overflowBuffer); + memcpy(state, staged, size); } - void finalizeState(uint8_t* state) const { return finalizeFunc(state); } + void finalizeState(uint8_t* state) const { + if (detail::isAggregateStateAligned(state)) { + return finalizeFunc(state); + } + auto size = static_cast(getAggregateStateSize()); + auto* staged = detail::stageAggregateState(state, size, 0); + finalizeFunc(staged); + memcpy(state, staged, size); + } bool isFunctionDistinct() const { return isDistinct; } diff --git a/src/processor/operator/persistent/reader/parquet/interval_column_reader.cpp b/src/processor/operator/persistent/reader/parquet/interval_column_reader.cpp index 8f8e05974a..e177e2c412 100644 --- a/src/processor/operator/persistent/reader/parquet/interval_column_reader.cpp +++ b/src/processor/operator/persistent/reader/parquet/interval_column_reader.cpp @@ -1,14 +1,22 @@ #include "processor/operator/persistent/reader/parquet/interval_column_reader.h" +#include + namespace lbug { namespace processor { common::interval_t IntervalValueConversion::readParquetInterval(const char* input) { + // The parquet plain-data buffer has no alignment guarantee, so copy each field out with + // memcpy instead of dereferencing a possibly-misaligned uint32_t* + // (UBSan: "load of misaligned address"). common::interval_t result; - auto inputData = reinterpret_cast(input); - result.months = inputData[0]; - result.days = inputData[1]; - result.micros = int64_t(inputData[2]) * 1000; + uint32_t months, days, microsLow; + memcpy(&months, input, sizeof(uint32_t)); + memcpy(&days, input + sizeof(uint32_t), sizeof(uint32_t)); + memcpy(µsLow, input + 2 * sizeof(uint32_t), sizeof(uint32_t)); + result.months = months; + result.days = days; + result.micros = int64_t(microsLow) * 1000; return result; } diff --git a/src/processor/result/base_hash_table.cpp b/src/processor/result/base_hash_table.cpp index 10d5dec997..955706fe7e 100644 --- a/src/processor/result/base_hash_table.cpp +++ b/src/processor/result/base_hash_table.cpp @@ -1,6 +1,7 @@ #include "processor/result/base_hash_table.h" #include +#include #include "common/constants.h" #include "common/null_buffer.h" @@ -79,8 +80,13 @@ static ft_compare_function_t getFactorizedTableCompareEntryFunc(const LogicalTyp template<> bool factorizedTableCompareEntry(const uint8_t* entry1, const uint8_t* entry2, const LogicalType& type) { - const auto* list1 = reinterpret_cast(entry1); - const auto* list2 = reinterpret_cast(entry2); + // entries point into packed factorized-table tuples without alignment padding, so they + // may be misaligned for list_t (8-byte alignment). Copy through aligned temporaries. + list_t list1Copy, list2Copy; + memcpy(&list1Copy, entry1, sizeof(list_t)); + memcpy(&list2Copy, entry2, sizeof(list_t)); + const auto* list1 = &list1Copy; + const auto* list2 = &list2Copy; if (list1->size != list2->size) { return false; } @@ -164,7 +170,12 @@ template<> uint32_t vectorPos, const uint8_t* entry) { auto dataVector = ListVector::getDataVector(vector); auto listToCompare = vector->getValue(vectorPos); - auto listEntry = reinterpret_cast(entry); + // `entry` points into a packed factorized-table tuple without alignment padding, so it + // may be misaligned for list_t (8-byte alignment). Copy through an aligned temporary + // (memcpy has no alignment requirement) instead of touching members in place. + list_t listEntryCopy; + memcpy(&listEntryCopy, entry, sizeof(list_t)); + auto listEntry = &listEntryCopy; auto entryNullBytes = reinterpret_cast(listEntry->overflowPtr); auto entryValues = entryNullBytes + NullBuffer::getNumBytesForNullValues(listEntry->size); auto rowLayoutSize = LogicalTypeUtils::getRowLayoutSize(dataVector->dataType);