Fix #46: size the rejection-sampling mask without float overflow - #95
Open
corgab wants to merge 2 commits into
Open
Fix #46: size the rejection-sampling mask without float overflow#95corgab wants to merge 2 commits into
corgab wants to merge 2 commits into
Conversation
integer() derived the bit count from ceil(log(range + 1, 2)) and the mask from 1 << bits. Both break near the top of the 64-bit range: the float logarithm under-counts for ranges such as 2^62, so the top value could never be drawn, and 1 << 63 overflows to a negative float, so integer(0, PHP_INT_MAX) used a corrupted mask. The bit length now comes from decbin(), which is exact, the 63-bit mask is PHP_INT_MAX itself, and a span that overflows the subtraction is rejected with a clear InvalidArgumentException instead of reaching bindec() as a float. Closes #46
… mask PHPStan types int - int as int, so a post-hoc is_int() check reads as always true; the guard now compares the bounds against PHP_INT_MAX + $min before subtracting, which also removes the reliance on float promotion. The mask was a no-op since every chunk is exactly $bitsNeeded digits, so bindec() alone yields the value. The new tests sit under their own banner.
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
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.
Summary
EntropyGenerator::integer()sized its rejection-sampling chunks with(int) ceil(log($range + 1, 2))and masked them with(1 << $bitsNeeded) - 1. Two things break near the top of the 64-bit range:(1 << 63) - 1overflows to a negative float, sointeger(0, PHP_INT_MAX)used a corrupted mask, as the issue reports.range = 2^62,2^62 + 1rounds to2^62as a double,log2gives exactly 62, and only 62 bits were drawn, so the top value of the range could never be produced. Verified:(int) ceil(log(2**62 + 1, 2))is 62 while the range needs 63 bits. The issue's exhaustive check missed this because it covered ranges of the form2^k - 1.Plan
Derive the bit count with integer arithmetic, drop the mask (every chunk is exactly
$bitsNeededbinary digits, sobindec()is already in range), and reject the one input the algorithm cannot serve: a span that overflows the subtraction, which would need a 64-bit chunk thatbindec()returns as a float.Changes
EntropyGenerator::integer(): the overflow is detected before subtracting ($min < 0 && $max > PHP_INT_MAX + $min), which keeps$rangeanintfor PHPStan and avoids relying on float promotion;$bitsNeeded = strlen(decbin($range)); the mask is gone. Docblock states the accepted bounds.The new guard throws
\InvalidArgumentException, like the two argument checks already in this class (min > max,bits < 1): these are caller mistakes on plain integer arguments, not package failures, so they stay outside theAetherExceptionhierarchy on purpose.No config change, no new dependencies, no change for ordinary ranges (the bit length is identical below 2^53 and the mask never altered a value).
Tests
EntropyGeneratorTest, under a "Wide ranges" banner:integer(0, PHP_INT_MAX)returnsPHP_INT_MAXfrom an all-ones chunk;integer(0, 2 ** 62)returns2 ** 62from a chunk of1followed by zeros (fails onmain, which returns2 ** 61);integer(PHP_INT_MIN, PHP_INT_MAX)throws before touching the device. The guard was also checked by hand on(PHP_INT_MIN, 0),(PHP_INT_MIN, -1),(0, PHP_INT_MAX)and(-1, PHP_INT_MAX).Local: pint passed, pest 794 tests passed.
Closes #46