Skip to content

test: Replace file level integer overflow suppression with function level suppression #21599

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
Apr 5, 2021
Merged

test: Replace file level integer overflow suppression with function level suppression #21599

merged 1 commit into from
Apr 5, 2021

Conversation

practicalswift
Copy link
Contributor

Replace file level (txmempool.cpp) signed integer overflow suppression with function level suppression (CTxMemPool::PrioritiseTransaction). The suppression was added yesterday in #21586.

Rationale: To avoid risk hiding other signed integer overflows in txmempool.cpp.

Obviously it would be better if this signed integer overflow fixed instead of suppressed - see details #20626. Any taker? :)

To hit the issue via fuzzing:

$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=validation_load_mempool src/test/fuzz/fuzz
INFO: Seed: 1184244493
INFO: Loaded 1 modules   (634418 inline 8-bit counters): 634418 [0x55a09fdfbf98, 0x55a09fe96dca),
INFO: Loaded 1 PC tables (634418 PCs): 634418 [0x55a09fe96dd0,0x55a0a08450f0),
INFO:     1264 files found in mempool/
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1040698 bytes
INFO: seed corpus: files: 1264 min: 1b max: 1040698b total: 15997133b rss: 197Mb
txmempool.cpp:847:15: runtime error: signed integer overflow: -7211388903327006720 + -7211353718954917888 cannot be represented in type 'long'
    #0 0x55a09c3ce2d8 in CTxMemPool::PrioritiseTransaction(uint256 const&, long const&) /home/thomas/bitcoin/src/txmempool.cpp:847:15

@JeremyRubin
Copy link
Contributor

JeremyRubin commented Apr 4, 2021 •
edited
Loading

utACK 585854a

@DrahtBot DrahtBot added the Tests label Apr 4, 2021
hebasto
hebasto approved these changes Apr 4, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 585854a, I have reviewed the code and it looks OK, I agree it can be merged.

@fanquake fanquake changed the title test: Replace file level (txmempool.cpp) signed integer overflow suppression with function level suppression (CTxMemPool::PrioritiseTransaction) test: Replace file level integer overflow suppression with function level suppression Apr 5, 2021
@maflcko
Copy link
Member

maflcko commented Apr 5, 2021

ACK, I forgot this is possible because all other suppressions use files

@maflcko maflcko merged commit 824eea5 into bitcoin:master Apr 5, 2021
@maflcko
Copy link
Member

maflcko commented Apr 5, 2021

Sorry for the edit war, but I reverted this in #21604 because due to a bug in clang we can't use symbol names and only file names.

@practicalswift practicalswift deleted the remove-blanket-signed-integer-overflow-suppression-from-txmempool branch April 10, 2021 19:43
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@practicalswift @JeremyRubin @maflcko @hebasto @DrahtBot