Skip to content

fix: prevent int overflow in Base58 length helpers - #10

Open
unsafePtr wants to merge 1 commit into
masterfrom
fix/length-overflow
Open

fix: prevent int overflow in Base58 length helpers#10
unsafePtr wants to merge 1 commit into
masterfrom
fix/length-overflow

Conversation

@unsafePtr

@unsafePtr unsafePtr commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Problem

n * K was computed in int at six places, so large inputs wrapped negative. The negative size then reached stackalloc, killing the process with an uncatchable StackOverflowException — reachable from a single public API call:

Entry point Crashes at
Base58.Bitcoin.Decode 2,929,719 chars
Base58.Bitcoin.Encode (both overloads) 15,675,064 bytes

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

  • Widen the arithmetic to long in both helpers, so the intermediate cannot wrap.
  • GetMaxEncodedLength throws ArgumentOutOfRangeException when the result would exceed int.MaxValue (above 1,567,506,311 bytes). Returning a clamped value would hand back a buffer size that cannot hold the output.
  • GetTypicalDecodedLength needs no range check — it shrinks its input, so the result always fits (max 1,574,105,514, with 573M headroom).
  • Collapse the four duplicated formulas onto the two helpers, so there is one definition of each bound.

Behaviour changes

  • GetMaxEncodedLength now uses 137/100 instead of 138/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.
  • GetMaxEncodedLength can 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 every n iff 58^137 > 2^800. Holds; 136 fails.
  • 733 = ceil(1000 * log(58)/log(256)) — sufficient iff 2^5864 > 58^1000. Holds; 732 fails.

The + 1 is load-bearing in both (at n=1, floor(1.37) = 1 but 2 digits are needed). These matter because the carry loops in ComputeGenericDigits / ComputeGenericDecode write buffer[count++] unguarded — Bitcoin Core guards its equivalent loop, we don't — so the bound is the only thing preventing an overrun. An undersized constant throws IndexOutOfRangeException below ~186 bytes (exact-size stackalloc) and silently truncates above it (ArrayPool.Rent over-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> on GetMaxEncodedLength.

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 int arithmetic or a constant is shrunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant