Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix common.h Memory Alignment Issue #275

Merged
merged 4 commits into from
Jan 21, 2025
Merged

Conversation

JaredTate
Copy link

This PR fixes a memory alignment issue detected by compiling with built in address sanitizer. It appears this file was not properly merged during the 8.22 process as BTC core had already fixed this. This is an important file used throughout the entire codebase.

The older file led to undefined behavior when ptr is not guaranteed to be 4-byte-aligned (for a uint32_t), or 8-byte-aligned (for a uint64_t). Many compilers and platforms allow unaligned reads if you compile with certain flags, but the C++ standard does not guarantee it is safe—and AddressSanitizer flags it.

Although on many x86 processors unaligned loads won’t crash at runtime, the C/C++ standard still considers it undefined behavior. UBSan/ASan is telling you that you are violating strict aliasing/alignment guarantees.

The simplest fix is to replace & use memcpy.

To discover this I compiled with this setting for ./configure:

./configure CXXFLAGS="-g -O1 -fno-omit-frame-pointer" --enable-debug --with-sanitizers=address,undefined

Screenshot 2025-01-21 at 9 38 34 AM

Running the c++ tests shows this fail right away:
Screenshot 2025-01-21 at 9 50 13 AM

Applying these fixes, re-running it no longer fails. Wallet compiles, runs and without sanitizer all 470 tests pass. However there are other issues with memory still to fix.

Screenshot 2025-01-21 at 10 08 56 AM

Note: This flow works well for memory debugging:

  1. Compile with ./configure + sanitizer checks
  2. Run c++ tests with: src/test/test_digibyte
  3. Read out put & fix bugs
  4. Rerun tests to confirm no issues
  5. Recompile with ./configure alone without sanitizer
  6. Re run src/test/test_digibyte and make sure all 470 tests pass
  7. Compile Qt, open let it run to make sure all wallet behaviour is normal

This is undefined behavior when ptr is not guaranteed to be 4-byte-aligned (for a uint32_t), or 8-byte-aligned (for a uint64_t). Many compilers and platforms allow unaligned reads if you compile with certain flags, but the C++ standard does not guarantee it is safe—and AddressSanitizer flags it.
This is undefined behavior when ptr is not guaranteed to be 4-byte-aligned (for a uint32_t), or 8-byte-aligned (for a uint64_t). Many compilers and platforms allow unaligned reads if you compile with certain flags, but the C++ standard does not guarantee it is safe—and AddressSanitizer flags it.
@JaredTate JaredTate changed the title Fix common.h Memory Alignment Bug Fix common.h Memory Alignment Issue Jan 21, 2025
Copy link
Member

@gto90 gto90 left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Member

@ycagel ycagel left a comment

Choose a reason for hiding this comment

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

cACK

@ycagel ycagel merged commit c5d9a01 into DigiByte-Core:develop Jan 21, 2025
7 checks passed
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.

3 participants