fix: prevent int overflow in Base58 length helpers - #10
Open
unsafePtr wants to merge 1 commit into
Open
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
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.
Problem
n * Kwas computed inintat six places, so large inputs wrapped negative. The negative size then reachedstackalloc, killing the process with an uncatchableStackOverflowException— reachable from a single public API call:Base58.Bitcoin.DecodeBase58.Bitcoin.Encode(both overloads)Root cause was duplication: the same two formulas were written out at four internal call sites in addition to the two public helpers, so the helpers and the code that actually sized the buffers could drift — and did.
Fix
longin both helpers, so the intermediate cannot wrap.GetMaxEncodedLengththrowsArgumentOutOfRangeExceptionwhen the result would exceedint.MaxValue(above 1,567,506,311 bytes). Returning a clamped value would hand back a buffer size that cannot hold the output.GetTypicalDecodedLengthneeds no range check — it shrinks its input, so the result always fits (max 1,574,105,514, with 573M headroom).Behaviour changes
GetMaxEncodedLengthnow uses137/100instead of138/100, matching what the internal encode paths already used. Both are valid upper bounds; 137 is the tighter one, so returned values shrink by ~1 byte per 230 (e.g. 32 bytes → 44 instead of 45). Still a sufficient bound at every length, so existing buffer-sizing code stays correct.GetMaxEncodedLengthcan now throw for very large inputs where it previously returned a wrapped negative.On the constants
Both are the minimal safe values, verifiable by integer identity with no floating point:
137 = ceil(100 * log(256)/log(58))— sufficient for everyniff58^137 > 2^800. Holds;136fails.733 = ceil(1000 * log(58)/log(256))— sufficient iff2^5864 > 58^1000. Holds;732fails.The
+ 1is load-bearing in both (atn=1,floor(1.37) = 1but 2 digits are needed). These matter because the carry loops inComputeGenericDigits/ComputeGenericDecodewritebuffer[count++]unguarded — Bitcoin Core guards its equivalent loop, we don't — so the bound is the only thing preventing an overrun. An undersized constant throwsIndexOutOfRangeExceptionbelow ~186 bytes (exact-sizestackalloc) and silently truncates above it (ArrayPool.Rentover-allocates). Both failure modes were confirmed by temporarily mutating the constants and observing the real encoder and decoder.The reasoning is recorded in the
<remarks>onGetMaxEncodedLength.Testing
Full suite green in Debug and Release: 141 tests, 0 failed, 0 warnings.
No test covers the overflow boundaries or the constant bounds, so nothing will fail if these helpers are later reverted to
intarithmetic or a constant is shrunk.