Skip to content

Conversation

@pierreN
Copy link
Contributor

@pierreN pierreN commented Apr 1, 2020

Add a fuzzing harness for CScriptNum::serialize, core_write.cpp:ValueFromAmount and util/moneystr.cpp:FormatMoney.

Those functions manually compute absolute values of int64_t numbers which can lead to undefined behavior, see : #18413 #18046

You can trigger this new harness with the following input :

$ echo -n "-9223372036854775808" > crash-abs-value
$ ./src/test/fuzz/abs_ub crash-abs-value

Note that BitcoinUnits::format also does the same (but requires QT headers to compile so I'm not sure we can add it to the fuzzer).

@practicalswift
Copy link
Contributor

@pierreN Thanks for contributing to the fuzzers.

ValueFromAmount is already fuzzed in src/test/fuzz/integer.cpp.

FormatMoney is already fuzzed in src/test/fuzz/integer.cpp.

That leaves CScriptNum::serialize(…) which isn't fuzzed directly today AFAICT.

Instead of adding a new fuzzing file just for CScriptNum::serialize(…) I suggest adding the line CScriptNum::serialize(i64) to the existing src/test/fuzz/integer.cpp. WDYT? :)

@pierreN pierreN changed the title tests: add fuzing harness for custom abs value functions tests: add CScriptNum::serialize to integer fuzz harness Apr 1, 2020
@pierreN
Copy link
Contributor Author

pierreN commented Apr 1, 2020

Ow, my bad. Thanks, I updated the PR accordingly.

@practicalswift
Copy link
Contributor

ACK 7a21407

@pierreN
Copy link
Contributor Author

pierreN commented Apr 2, 2020

Thanks. Note that tests fail since #18413 is not merged AFAIK.

@maflcko
Copy link
Member

maflcko commented Apr 2, 2020

Thanks. Note that tests fail since #18413 is not merged AFAIK.

Could add the tests to that pull and close this one?

@pierreN pierreN closed this Apr 2, 2020
@pierreN
Copy link
Contributor Author

pierreN commented Apr 2, 2020

Yes good idea, I just added the test to the original PR : c6819c4

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants