Skip to content

[fuzz] Avoid partial negative result #29462

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
achow101 merged 1 commit into from
Feb 22, 2024
Merged

[fuzz] Avoid partial negative result #29462

achow101 merged 1 commit into from
Feb 22, 2024

Conversation

@murchandamus
Copy link
Contributor

@murchandamus murchandamus commented Feb 21, 2024 •
edited by fanquake
Loading

May address the problem reported by maflcko in #27877 (review).

For some values, MAX_MONEY - max_spendable - max_output_groups could result in a partial negative value. By putting the addition of group_pos.size() first, all partial results in this line will be strictly positive.

I opened this as a draft, since I was unable to reproduce the issue, so I’m waiting for confirmation whether this in fact mitigates the problem.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 21, 2024 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko, sipa, brunoerg, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@murchandamus
Copy link
Contributor Author

murchandamus commented Feb 21, 2024

Opening for review:
After @achow101 pointed out that I needed to enable the "integer" fuzz sanitizer, I was able to reproduce the issue and verify that the proposed fix mitigates the problem.

@murchandamus murchandamus marked this pull request as ready for review February 21, 2024 21:40
@murchandamus
Copy link
Contributor Author

murchandamus commented Feb 21, 2024

Opening for review:
After @achow101 pointed out that I needed to enable the "integer" fuzz sanitizer, I was able to reproduce the issue and verify that the proposed fix mitigates the problem.

@achow101 achow101 added this to the 27.0 milestone Feb 21, 2024
@maflcko
Copy link
Member

maflcko commented Feb 22, 2024

Tested with:

./autogen.sh && CC=clang CXX=clang++ ./configure -q --enable-c++20 --enable-fuzz --with-sanitizers=fuzzer,undefined,integer,float-divide-by-zero && make clean && make

and #27877 (comment) to confirm the fix

ACK 9dae3b9

@sipa
Copy link
Member

sipa commented Feb 22, 2024

utACK 9dae3b9

brunoerg
brunoerg approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

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

crACK 9dae3b9

@achow101
Copy link
Member

achow101 commented Feb 22, 2024

ACK 9dae3b9

@achow101 achow101 merged commit 1ac627c into bitcoin:master Feb 22, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Feb 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@brunoerg brunoerg brunoerg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

27.0

Development

Successfully merging this pull request may close these issues.

6 participants

@murchandamus @DrahtBot @maflcko @sipa @achow101 @brunoerg