Skip to content

Decode the table-free character sets without encoding support - #110

Open
justanothertool wants to merge 1 commit into
rxing-core:mainfrom
justanothertool:no-charset-support-decodes
Open

justanothertool wants to merge 1 commit into
rxing-core:mainfrom
justanothertool:no-charset-support-decodes

Conversation

@justanothertool

Copy link
Copy Markdown

Fixes #109.

With no_character_set_support, CharacterSet::encode, encode_replace and decode were unimplemented!(), so the first byte-mode segment panicked — every QR code carrying a URL.

This makes the feature handle what the standard library converts without lookup tables: UTF-8, UTF-16, UTF-32, US-ASCII and ISO-8859-1 (also used for Binary). Every other set returns an UnsupportedOperationException naming the feature it needs, so a reader treats it like any other undecodable symbol instead of panicking. decode_replace is added for parity with the other two backends.

  • The table-free conversions move into a private table_free module, which the encoding_rs backend now calls for the same sets. Its behaviour is unchanged: cargo test --workspace --release gives 784 passed, as on main.
  • New integration test tests/no_character_set_support.rs: QR round trips through the writer and the reader (ASCII, ISO-8859-1, UTF-8), a set that needs tables (Shift_JIS) is an error, and the conversions themselves. On main every one of them panics, except the decode_replace test, which does not build there. It is an integration test because the library's unit tests do not build without the default features (CI "Run tests (minimal)" runs with default features due to workspace feature unification #107).
  • CI runs it: cargo test -p rxing --release --no-default-features --features qrcode,decoders,encoders,no_character_set_support --test no_character_set_support.
  • The feature's doc comment in Cargo.toml says what still works.
  • It uses no standard library API newer than the 1.85 MSRV.

Size, for a binary that only decodes QR codes (x86-64, opt-level = "z", LTO, stripped): 678 kB with encoding_rs, 505 kB with no_character_set_support.

With `no_character_set_support`, `CharacterSet::encode`, `encode_replace`
and `decode` were `unimplemented!()`, so the first byte-mode segment
panicked. That is every QR code that carries a URL, which made the
feature unusable for its main use: a QR reader without the
`encoding_rs` tables, which are most of its size.

The feature now handles the sets the standard library converts without
lookup tables: UTF-8, UTF-16, UTF-32, US-ASCII and ISO-8859-1 (also
used for Binary). Every other set returns an error, which a reader
treats like any other undecodable symbol. `decode_replace` is added
for parity with the other two backends.

The table-free conversions move into a private `table_free` module
that the `encoding_rs` backend now calls for the same sets. Its
behaviour is unchanged; the default test suite passes as before.

The new tests round-trip QR codes through the writer and the reader
with the feature on, and check the conversions themselves. They run as
an integration test because the library's unit tests do not build
without the default features (rxing-core#107). CI gets a step that runs them.
@hschimke

Copy link
Copy Markdown
Collaborator

Thanks for the PR, I looked over it and it looks good.

I pointed Claude at it and it found one issue which I didn't: (below is entirely Claude)

Thanks for this. Swapping the unimplemented!() panics for real conversions makes no_character_set_support usable, and pulling the shared code into table_free keeps the encoding_rs path unchanged. The new tests and the full default suite pass for me, and every feature combination I tried compiles.

  1. Sets that need the lookup tables are silently dropped, not reported as errors

The description says other sets return “an error, which a reader treats like any other undecodable symbol.” For QR codes that isn’t what happens. ECIStringBuilder::encode_segment (src/common/eci_string_builder.rs:210) runs CharacterSet::from(eci).decode(bytes).ok()?, and encodeCurrentBytesIfAny then applies unwrap_or_default(). So the segment becomes "" and the decode still succeeds:

use rxing::common::{ECIStringBuilder, Eci};

let mut b = ECIStringBuilder::with_capacity(16);
b.append_bytes(b"before ");
b.append_eci(Eci::Shift_JIS);
b.append_bytes(b"\x82\xa0\x82\xa2");
b.append_eci(Eci::ISO8859_1);
b.append_bytes(b" after");
assert_eq!(b.to_string(), "before  after"); // Shift_JIS segment gone, no error

The default build already does this for malformed bytes. With this feature it happens to every valid segment in Shift_JIS, Cp1252, ISO-8859-x and the like. It also happens with no ECI at all, because decodeByteSegment takes whatever guessCharset returns, and that can be Shift_JIS. A reader that returns a successful result with text missing is worse than one that fails, especially for payloads like payment or enrolment codes.

Options I can see:

return an error from the builder or the decoder when a segment can’t be decoded, or
in encode_segment, fall through to the existing not_encoded_yet raw-byte fallback instead of returning early with ?, so the bytes at least come through as Latin-1.

If you’d rather keep this PR small, the description and the Cargo.toml feature comment should at least say that these segments are dropped.

  1. Nit: UTF-16 byte order mark isn’t stripped

decode_utf8 strips a leading UTF-8 BOM to match encoding_rs, but decode_utf16 doesn’t strip FE FF / FF FE. guessCharset picks UTF-16BE/LE precisely because those bytes are there, so in the no-tables build the decoded text starts with an invisible U+FEFF that the default build removes. Stripping the matching BOM in decode_utf16 would make the two builds behave the same.

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.

no_character_set_support panics when decoding a byte-mode QR code

2 participants