This PR is a tiny improvement for the bip324_cipher_roundtrip fuzz target: currently the damaging of input data for decryption (either ciphertext or aad) only ever happens in the lower nibble within the byte at the damage position, as the bit position for the damage_val byte was calculated with damage_bit & 3 (corresponding to % 4) rather than damage_bit & 7 (corresponding to the expected % 8).
Noticed while reviewing #28263 which uses similar constructs.
Currently the damaging of input data for decryption (either ciphertext
or aad) only ever happens in the lower nibble within the byte at the
damage position, as the bit position for the `damage_val` byte was
calculated with `damage_bit & 3` (corresponding to `% 4`) rather than
`damage_bit & 7` (corresponding to the expected `% 8`).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stratospher, dergoegge |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
97@@ -98,7 +98,7 @@ FUZZ_TARGET(bip324_cipher_roundtrip, .init=Initialize)
98 unsigned damage_bit = provider.ConsumeIntegralInRange<unsigned>(0,
99 (ciphertext.size() + aad.size()) * 8U - 1U);
100 unsigned damage_pos = damage_bit >> 3;
101- std::byte damage_val{(uint8_t)(1U << (damage_bit & 3))};
102+ std::byte damage_val{(uint8_t)(1U << (damage_bit & 7))};
nit: Maybe just use % instead? That reads a bit easier and probably would have avoided this in the first place.
0 std::byte damage_val{(uint8_t)(1U << (damage_bit % 8))};
Good point, I agree. In a similar sense, the damage position should probably be calculated as damage_bit / 8 instead of a right-shift by 3. I assume the original motivation for using “bit-fiddling” instead was performance (worries that the modulus operation is significantly slower), but it seems both variants emit the same code on recent compilers, even without optimization turned on: https://godbolt.org/z/EjY59a36s
Will leave the PR as it is now, as it has already two ACKs, but will consider working on a follow-up PR that replaces more constructs like this (i.e. division and bitwise AND with a powers-of-two constant) in our code-base.
Labels
Tests