-
Notifications
You must be signed in to change notification settings - Fork 138
block-buffer v0.10 #671
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
block-buffer v0.10 #671
Conversation
1af43dc
to
5cbb42c
Compare
98c84bc
to
2cb6089
Compare
buffer-buffer` removed the "block-padding" feature in this PR: RustCrypto/utils#671
// around compiler accounting for possible overflow of | ||
// `pos + n` which results in it inserting unreachable | ||
// panic branches. Using `unreachable_unchecked` in `get_pos` | ||
// we convince compiler that `BlockSize - pos` never underflows. |
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.
I believe you mean overflow here, not underflow? If BlockSize - pos < 0, that's still overflow. Underflow only applies to floating point arithmetic.
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.
https://cwe.mitre.org/data/definitions/191.html
CWE-191: Integer Underflow (Wrap or Wraparound)
The product subtracts one value from another, such that the result is less than the minimum allowable integer value, which produces a value that is not equal to the correct result.
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.
I guess it is saying specifically "integer underflow" and not "arithmetic underflow", but that's a bit of a confusing usage imo.
https://en.wikipedia.org/wiki/Arithmetic_underflow
Storing values that are too low in an integer variable (e.g., attempting to store −1 in an unsigned integer) is properly referred to as integer overflow, or more broadly, integer wraparound. The term underflow normally refers to floating point numbers only, which is a separate issue.
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're debugging a crash around here (actually in get_pos()
that only happens only on win x86 in release config. It started only recently so it may be related to rustc version and it's a bit odd because it appears to crash on https://github.com/RustCrypto/utils/pull/671/files#diff-dab38e6f4d05085a1ab6b7b064fb097fa39f1806d8b03dfbb80472aaa21851e0R171
I'll open a ticket for it if we can repro in a pure rust test
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.
I would say "integer underflow" has well-established usage, especially in the area of cryptography engineering.
That's a bold claim being made in Wikipedia, and I see it's completely uncited, so I would definitely take it with a grain of salt.
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.
it's also worth noting that chromium uses a nightly snapshot of rustc and does not use cargo
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.
Sorry, actually I take that back. I don't think we have tried a build that would trigger the debug asserts so we'll look at that as well. When I said "no crashes in debug" I meant that there are no crashes in other build configurations we use other than our official build config for win x86
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.
so we do see this with the debug assertions enabled
thread '<unnamed>' panicked at .\../../brave/third_party/rust/block_buffer/v0_10/crate/src/lib.rs:198:13:
assertion failed: false
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.
I still don't have a pure rust STR, but I'll open an issue #989
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.
btw - the panic message when we replicated this in rust was attempt to subtract with overflow
:)
Split-off from #566
Notable changes:
Eager
andLazy
buffer type parameters which are responsible for defining cursor position invariant. For theEager
kind cursor position is always smaller than block size (i.e. buffer always contains at least one unfilled byte), while forLazy
it's less or equal (i.e. buffer can contain a complete block) The kinds are used as an important part of thedigest
core API.block-padding
. It results in a bit of code duplication in the hash crates, but reduces coupling between our crates.u8
for cursor position, which saves 7 bytes of state size on 64-bit targets and relaxes alignment requirements.Right now it mostly tied to hash and MACs needs. I plan to experiment on cipher buffers separately inside the
cipher
crate. Cipher-related buffer functionality can be merged into this crate at a later date in separate versions.Closes #8
Closes #669