Skip to content

test: Add sanitizer suppressions for AMD EPYC CPUs #20844

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 6, 2021
Merged

test: Add sanitizer suppressions for AMD EPYC CPUs #20844

merged 1 commit into from
Jan 6, 2021

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jan 4, 2021

Currently the ci system only runs on intel cpus (and some arm devices), but it won't run on CPUs Using the 'shani(1way,2way)' SHA256 implementation (excerpt from debug log).

For reference, google cloud CPUs (which is what Cirrus CI uses) print Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation

The traceback I got:

crypto/sha256_shani.cpp:87:18: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned long')
    #0 0x55c0000e95ec in sha256_shani::Transform(unsigned int*, unsigned char const*, unsigned long) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256_shani.cpp:87:18
    #1 0x55bfffb926f8 in (anonymous namespace)::SelfTest() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:517:9
    #2 0x55bfffb906ed in SHA256AutoDetect[abi:cxx11]() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:626:5
    #3 0x55bfff87ab97 in BasicTestingSetup::BasicTestingSetup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:104:5
    #4 0x55bffe885877 in main /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_main.cpp:52:27
    #5 0x7f20c3bf60b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #6 0x55bffe7a5f6d in _start (/root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_bitcoin-qt+0x1d00f6d)

SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow crypto/sha256_shani.cpp:87:18 in

@fanquake fanquake added the Tests label Jan 4, 2021
@laanwj
Copy link
Member

laanwj commented Jan 6, 2021

I think the unsigned integer overflow check is questionable in the first place. After all, unsigned integer overflow isn't UB and completely normal in cryptography and other bit manipulation code.

Anyhow ACK fa6c114

@laanwj laanwj merged commit e520e09 into bitcoin:master Jan 6, 2021
@maflcko maflcko deleted the 2101-testEpycSan branch January 6, 2021 07:41
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 6, 2021
@laanwj
Merge bitcoin#20844: test: Add sanitizer suppressions for AMD EPYC CPUs
7edd17c 
fa6c114 test: Add sanitizer suppressions for AMD EPYC CPUs (MarcoFalke)

Pull request description:

  Currently the ci system only runs on intel cpus (and some arm devices), but it won't run on CPUs `Using the 'shani(1way,2way)' SHA256 implementation` (excerpt from debug log).

  For reference, google cloud CPUs (which is what Cirrus CI uses) print `Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation`

  The traceback I got:

  ```
  crypto/sha256_shani.cpp:87:18: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned long')
      #0 0x55c0000e95ec in sha256_shani::Transform(unsigned int*, unsigned char const*, unsigned long) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256_shani.cpp:87:18
      #1 0x55bfffb926f8 in (anonymous namespace)::SelfTest() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:517:9
      #2 0x55bfffb906ed in SHA256AutoDetect[abi:cxx11]() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:626:5
      #3 0x55bfff87ab97 in BasicTestingSetup::BasicTestingSetup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:104:5
      #4 0x55bffe885877 in main /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_main.cpp:52:27
      #5 0x7f20c3bf60b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
      #6 0x55bffe7a5f6d in _start (/root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_bitcoin-qt+0x1d00f6d)

  SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow crypto/sha256_shani.cpp:87:18 in

ACKs for top commit:
  laanwj:
    Anyhow ACK fa6c114

Tree-SHA512: 968a1d28eedec58c337b1323862f583cb1bcd78c5f03396940b9ab53ded12f8c6652877909aba05ee5586532137418fd817ff979bd7bef6e07856094f9d7f9b1
@practicalswift
Copy link
Contributor

practicalswift commented Jan 10, 2021 •
edited
Loading

Post-merge ACK fa6c114: a general crypto/* suppression for unsigned-integer-overflow makes perfect sense!

Thanks for fixing this @MarcoFalke!

@laanwj

I think the unsigned integer overflow check is questionable in the first place. After all, unsigned integer overflow isn't UB and completely normal in cryptography and other bit manipulation code.

In fairness it is probably worth mentioning that the goal of the UBSan's unsigned-integer-overflow is not to catch UB: as you correctly point out there is no UB to catch when it comes to unsigned integers :)

The idea is instead to make reliance on wraparound semantics explicit by documenting in which areas of our code code we expect unsigned integer wraparounds to occur (such as in crypto/*).

UBSan can then flag all wraparounds outside of the expected areas: such cases are are likely unintentional (bugs).

Also, it should be noted that the list of unsigned-integer-overflow suppressions in test/sanitizer_suppressions/ubsan has remained largely static over time which suggests that this UBSan check is a very cheap way to kill this bug class (unintended unsigned integer wraparounds).

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maflcko @laanwj @practicalswift @fanquake