Skip to content

Fix Base36 round-trip for empty, all-zero, and high-bit-set inputs (fixes #67) - #72

Open
youdie006 wants to merge 1 commit into
multiformats:masterfrom
youdie006:fix/67-base36-empty-and-leading-zero-roundtrip
Open

Fix Base36 round-trip for empty, all-zero, and high-bit-set inputs (fixes #67)#72
youdie006 wants to merge 1 commit into
multiformats:masterfrom
youdie006:fix/67-base36-empty-and-leading-zero-roundtrip

Conversation

@youdie006

Copy link
Copy Markdown

What

Base36.encode/decode route bytes through BigInteger, which loses fidelity two ways, so decode(encode(x)) does not return x:

  1. Empty / all-zero (reported in Base36 fails round trip encoding of empty byte arrays #67). new BigInteger(1, allZeros).toString(36) yields "0" not "", and new BigInteger("0", 36).toByteArray() yields [0] not an empty array. Each side adds a spurious byte/char and they compound over a round trip: [] -> "0" -> [0, 0].
  2. Sign byte (latent, not in the report). For a value whose top byte has the high bit set, BigInteger.toByteArray() prepends a 0x00 two's-complement sign byte that decode never strips: [0x80] -> "3k" -> [0x00, 0x80]. This affects roughly half of all inputs.

Both are reachable through the public Multibase API, which routes the Base36/Base36Upper codes (k/K) straight to these methods.

Fix

Mirror the contract of this repo's own Base58 (empty -> empty, leading zeros preserved, no sign byte), keeping the existing leading-zero-prefix logic:

  • encode: emit "" for a zero value instead of "0".
  • decode: return new byte[0] for empty input, and strip a single leading 0x00 sign byte from toByteArray() for a non-zero value.

For a positive BigInteger, toByteArray() has at most one leading 0x00 (the sign byte), and only when the top magnitude byte's high bit is set, so stripping one is always safe; genuine leading-zero bytes of the input are still carried by the separate zeroPrefixLength path.

Tests

New Base36Test (JUnit 5, matching the existing suite):

  • size-preserving round trip for {}, {0}, {0,0}, {0x80}, {0x01};
  • empty <-> "";
  • the pre-existing multibase base36 vectors stay byte-identical, and the leading-zero vector still decodes to 0x00 ++ decode(body);
  • an exhaustive round trip over every array of length 0..2.

The exhaustive check goes from 33027 of 65793 inputs failing (before) to 0 (after). Pre-existing MultibaseTest (108) and MultibaseBadInputsTest (9) stay green; 121 tests pass total.

Thanks to @hossman for the clear report and repro.


This change was prepared with AI assistance and reviewed by me before submission.

Base36.encode/decode route bytes through BigInteger, which loses fidelity two
ways so decode(encode(x)) does not return x: (1) the empty/all-zero case maps to
"0"/[0] instead of ""/[], compounding over a round trip ([] -> "0" -> [0,0]);
(2) BigInteger.toByteArray() prepends a 0x00 two's-complement sign byte for any
value whose top byte has the high bit set, which decode never strips
([0x80] -> "3k" -> [0x00,0x80]), affecting roughly half of all inputs. Both are
reachable through the public Multibase API (k/K codes).

Mirror the contract of this repo's own Base58 (empty -> empty, leading zeros
preserved, no sign byte): encode emits "" for a zero value, and decode returns
an empty array for empty input and strips a single leading 0x00 sign byte for a
non-zero value.

Fixes multiformats#67.
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