Skip to content

fix(c++): fix misaligned address access errors detected by UBSan in buffer.h#3479

Open
BaldDemian wants to merge 2 commits intoapache:mainfrom
BaldDemian:ub-fix
Open

fix(c++): fix misaligned address access errors detected by UBSan in buffer.h#3479
BaldDemian wants to merge 2 commits intoapache:mainfrom
BaldDemian:ub-fix

Conversation

@BaldDemian
Copy link

@BaldDemian BaldDemian commented Mar 14, 2026

Why?

buffer.h reads and writes multi-byte integers (16/32/64-bit) directly into a raw uint8_t* buffer. The original code cast the byte pointer to a typed pointer and dereferenced it:

  • Read:
    return reinterpret_cast<const T*>(data_ + offset)[0];

  • Write:
    *reinterpret_cast<T*>(data_ + offset) = value;

Dereferencing a pointer that is not aligned to alignof(T) is considered UB in C++. UBSan correctly flagged these as misaligned address runtime errors, because data_ + offset can be at any byte boundary.

What does this PR do?

Two helper templates were added to the buffer.h:

template <typename T>
FORY_ALWAYS_INLINE static T load_unaligned(const uint8_t *ptr) {
    T value;
    std::memcpy(&value, ptr, sizeof(T));
    return value;
}

template <typename T>
FORY_ALWAYS_INLINE static void store_unaligned(uint8_t *ptr, T value) {
    std::memcpy(ptr, &value, sizeof(T));
}

All reinterpret_cast calls that may lead to UB in the file were replaced with calls to these helpers.

No UB were detected when running bazel test --cache_test_results=no --config=x86_64 --config=ubsan $(bazel query //...) after applying this patch.
Details can be found in ubsan_report.txt.
Only a few unused-but-set-parameter warnings were detected by UBSan.

Related issues

Fix #3459

AI Contribution Checklist

  • Substantial AI assistance was used in this PR: yes / no
  • If yes, I included a completed AI Contribution Checklist in this PR description and the required AI Usage Disclosure.

Does this PR introduce any user-facing change?

  • Does this PR introduce any public API change?
  • Does this PR introduce any binary protocol compatibility change?

Benchmark

I believe replacing reinterpret_cast with memcpy won't incur much runtime burden, since memcpy can be optimized by both GCC and Clang effectively.

@chaokunyang
Copy link
Collaborator

Does thos fix affect the performance? Could you run benchmarks/cpp And compare the performance

@BaldDemian
Copy link
Author

BaldDemian commented Mar 14, 2026

after_README.md
before_README.md
Hey @chaokunyang, here are the cpp benchmark results after and before this fix. From my understanding, there are no major fluctuations in the performance 🤔 (mostly within ±3%, with a maximum around 8%)

(in benchmarks/cpp, run ./run.sh --serializer fory)

@BaldDemian
Copy link
Author

run ./fory_benchmark --benchmark_filter="Fory" in benchmarks/cpp/build.
Before the commit:
before
After the commit:
after

Still I don't think there is regression in the performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UBSan runtime error: load/store of misaligned address

2 participants