Fix common.h Memory Alignment Issue #275
Merged
Conversation
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
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
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:
Running the c++ tests shows this fail right away:
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.
Note: This flow works well for memory debugging:
src/test/test_digibytesrc/test/test_digibyteand make sure all 470 tests pass