random: Fix off-by-one in fast path selection of Randomizer::getBytesFromString() #10449
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…FromString() With a single byte we can choose offsets between 0x00 and 0xff, thus 0x100 different offsets. We only need to use the slow path for sources of more than 0x100 bytes. The previous version was correct with regard to the output expectations, it was just slower than necessary. Better fix this now while we still can before being bound by our BC guarantees with regard to emitted sequences. This also adds a test to verify the behavior: For powers of two we never reject any values during rejection sampling, we just need to mask off the unneeded bits. Thus we can specifically verify that the number of calls to the engine match the expected amount. We also verify that all the possible values are emitted to make sure the masking does not remove any required bits. For inputs longer than 0x100 bytes we need trust the `range()` implementation to be unbiased, but still verify the number of engine calls and perform a basic output check.
TimWolla
force-pushed
the
get-bytes-from-string
branch
from
ee7a5bb to
6973244
Compare
| echo PHP_EOL; | ||
|
|
||
| // Fast path: Inputs less than or equal to 256. | ||
| for ($i = 1; $i <= \strlen($allBytes); $i *= 2) { |
There was a problem hiding this comment.
Is there a reason here that you use the FQN for the function and not above (same as line 37 and 60). This seems inconsistent.
There was a problem hiding this comment.
Good catch, no, there is not. My muscle memory code style has the leading backslash, but the tests do not usually use it. I'll remove it.
Girgias
approved these changes
Jan 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With a single byte we can choose offsets between 0x00 and 0xff, thus 0x100 different offsets. We only need to use the slow path for sources of more than 0x100 bytes.
The previous version was correct with regard to the output expectations, it was just slower than necessary. Better fix this now while we still can before being bound by our BC guarantees with regard to emitted sequences.
This also adds a test to verify the behavior: For powers of two we never reject any values during rejection sampling, we just need to mask off the unneeded bits. Thus we can specifically verify that the number of calls to the engine match the expected amount. We also verify that all the possible values are emitted to make sure the masking does not remove any required bits. For inputs longer than 0x100 bytes we need trust the
range()implementation to be unbiased, but still verify the number of engine calls and perform a basic output check./cc @joshuaruesweg