-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Reject non-canonical binaries #6277
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.
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
Reject non-canonical binaries #6277
Conversation
src/test/app/HostFuncImpl_test.cpp
Outdated
| } | ||
|
|
||
| void | ||
| testFloatNonCanonical() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we combine testFloatNonIOU and testFloatNonCanonical into something like testFloatSpecialCases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved
src/test/app/HostFuncImpl_test.cpp
Outdated
| void | ||
| testFloatNonCanonical() | ||
| { | ||
| testcase("float Xrp+Mpt"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ripple/wasmi-host-functions #6277 +/- ##
===========================================================
Coverage 79.9% 79.9%
===========================================================
Files 854 854
Lines 73782 73799 +17
Branches 8301 8301
===========================================================
+ Hits 58918 58935 +17
Misses 14864 14864
🚀 New features to boost your workflow:
|
e78687d to
89654d2
Compare
| std::string | ||
| floatToString(Slice const& data) | ||
| { | ||
| detail::FloatState rm(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Floats has global state for rounding and mantissa length. FloatState restore that state on return / exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's restored, why does it need to be instantiated here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We save current state of the Numbers during initialization, switch to our state and restore state on exit, so every float host-functions doesn't change system settings for floats. Isn't it obvious?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline.
It's done in the other functions, the other functions just take a mode param to signify the rounding mode used in the math. These two functions don't need a rounding mode since there is no computation done, so we can just use the regular rounding mode.
| { | ||
| try | ||
| { | ||
| detail::FloatState rm(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same here
c1c1b4e
into
XRPLF:ripple/wasmi-host-functions
High Level Overview of Change
Check and reject slices if they are non-canonical (which is possible if craft binary manually) but still represent valid number.
Type of Change
.gitignore, formatting, dropping support for older tooling)