Two encoder correctness fixes - #53
Open
tokyovigilante wants to merge 2 commits into
Open
Conversation
ISO/IEC 18004 §7.4.3 specifies that in numeric mode each complete group
of three digits is encoded in a fixed 10-bit binary value, and only a
trailing remainder of two or one digit uses the reduced 7-bit or 4-bit
widths.
The encoder instead selected the group width from the group's leading
digits (7 bits when the group began with a single '0', 4 bits when it
began with "00"). Any data containing a zero-led full group therefore
emitted too few bits and shifted the entire remaining bitstream, so a
value such as "0123456789" decoded to garbage. Upstream's test vector
"8675309" happens to contain no zero-led group, which is why the defect
went unnoticed.
Full groups are now always encoded in 10 bits; the 7/4-bit remainder
handling is unchanged. Adds regression vectors covering zero-led groups
("012", "00012") and a full-pipeline vector for "0123456789".
The QR capacity table in ISO/IEC 18004 is inclusive: a version's listed character capacity is the maximum number of characters it can hold. When searching for the smallest version, the loop compared the data length with `<`, so a payload whose length exactly matched a version's capacity was pushed to the next larger version, wasting a version. At the very top of the range the payload fell through the loop and was wrongly rejected with DataSizeDefect, making a maximal payload (7089 digits at v40-L) impossible to encode. Use `<=` so an exact-capacity payload fits in that version. The explicit `version` path in newQRCode already uses `>` correctly and is unchanged. Adds boundary regression tests at v1 and v40 capacities.
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
1 participant
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.
Found while generating QR codes for a sign-in flow. Two independent
commits with regression tests, based on 3.1.0
Numeric mode: full groups must be 10 bits.
encodeNumericModeDatapicks each group's width from its leading digits (7 bits for
0…, 4 for00…), but ISO/IEC 18004 §7.4.3 fixes every complete 3-digit group at10 bits — the reduced widths are only for a trailing 2- or 1-digit
remainder. A zero-led group emits too few bits and shifts everything
after it, so
"0123456789"scans as garbage. The existing"8675309"vector has no zero-led group, which is why it went unnoticed. Fix: always
10 bits for a full group.
getSmallestVersion: capacity is inclusive. The search uses<, so apayload exactly matching a version's capacity is bumped to the next
version — and at the top of the range there is no next version, so the
maximum payload (7089 digits at v40-L) falls through and is rejected with
DataSizeDefect. Fix:<=. The explicit-versionpath innewQRCodealready uses
>and is untouched.Tests: zero-led group vectors plus a full-pipeline
"0123456789", andcapacity boundaries at v1 and v40.
AI Disclosure: Mix of Claude Fable and local Qwen 3.8 models used to assist writing, PR reviewed and tested manually.