Skip to content

Fix dead ill-formed-fourth-byte UTF-8 test sections (byte3/byte4 typo) - #5499

Open
nlohmann wants to merge 2 commits into
developfrom
issue-5416-utf8-fourth-byte-typo
Open

nlohmann wants to merge 2 commits into
developfrom
issue-5416-utf8-fourth-byte-typo

Conversation

@nlohmann

@nlohmann nlohmann commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes #5416.

The SECTION("ill-formed: wrong fourth byte") blocks in tests/src/unit-unicode3.cpp, tests/src/unit-unicode4.cpp, and tests/src/unit-unicode5.cpp guarded their byte4 sweep with a check on byte3 instead of byte4:

for (int byte4 = 0x00; byte4 <= 0xFF; ++byte4)
{
    // skip correct fourth byte
    if (0x80 <= byte3 && byte3 <= 0xBF)   // <-- checks byte3, should be byte4
    {
        continue;
    }
    ...
}

Because the enclosing loop already restricts byte3 to 0x80..0xBF, this guard was always true, so continue fired unconditionally and check_utf8string()/check_utf8dump() were never actually called — a malformed fourth byte in a 4-byte UTF-8 sequence has never been tested. unit-unicode3.cpp also had a garbled comment ("skip fourth second byte"), fixed here as well.

Fix

Fixed the guard to check byte4 (and fixed the garbled comment in unit-unicode3.cpp), and kept the full byte2 x byte3 combinatorics — matching the style of the sibling "wrong second/third byte" sections in the same files, which also sweep every combination of the bytes they don't directly target. This is a deliberate choice: a smaller, representative-prefix version of this fix was considered (and briefly implemented) on the reasoning that the lexer's next_byte_in_range() validates continuation bytes strictly in sequence with early-exit, so byte4's accept/reject outcome doesn't depend on which specific valid byte2/byte3 values were used to reach it — but the maintainer prefers exhaustive coverage of every byte combination here for a stronger coverage claim, consistent with how the rest of the file already tests this property, so the full sweep is kept.

The companion issue #5418 (further runtime-reduction ideas: the wrong-2nd/3rd-byte sections' combinatorics, the binary-format 4x-parse issue, 16-bit integer sweeps, etc.) is intentionally left out of scope for a separate PR/decision.

Verification (done offline, without CMake/network)

For each file, compiled and ran with:

clang++ -std=c++11 -O1 -w -I include -I tests/thirdparty/doctest -I tests/src tests/src/unit-unicodeN.cpp -o /tmp/out && /tmp/out --no-skip -tce="*downloaded*"
  • Proved the section was dead code: on the pre-fix code, running just the "wrong fourth byte" subcase (via doctest's --subcase filter) executes 0 assertions in all three files.

  • Proved the fix is meaningful: deliberately reintroducing a bug in the lexer's byte4 range check for the 0xF0 case (temporarily widening it to accept any byte) causes the fixed unit-unicode3.cpp test to fail — confirming the test now actually catches a byte4 validation regression. The bug was reverted immediately after confirming the failure.

  • Full-suite assertion counts, before -> after (with the full combinatorial sweep restored):

    File Before (dead code) After (full sweep)
    unit-unicode3.cpp 19,501,644 26,579,532
    unit-unicode4.cpp 65,423,652 93,735,204
    unit-unicode5.cpp 14,889,164 17,248,460

    (unicode4's +28.3M matches the issue's own estimate of ~2.36M added iterations for a full-sweep fix.)

  • No regressions: all other sections in all three files still pass unchanged, and all three files run clean (0 failed) with --no-skip.

Breaking change?

No. This is a test-only change (no include/ changes) and does not affect the public API in any way.

— opened by Claude Code on behalf of @nlohmann

The "ill-formed: wrong fourth byte" SECTIONs in unit-unicode3.cpp,
unit-unicode4.cpp, and unit-unicode5.cpp guarded their loop with a check
on byte3 instead of byte4. Since the enclosing loop already restricts
byte3 to its valid range, the guard was always true and the section's
"continue" fired unconditionally, so check_utf8string()/check_utf8dump()
were never actually invoked for a malformed fourth byte.

Fixing the guard naively (byte3 -> byte4) would also have swept the full
byte2 x byte3 combinatorics for every byte4 value, adding millions of
redundant iterations: the lexer validates continuation bytes strictly in
sequence with early exit (see next_byte_in_range() in lexer.hpp), so once
byte2/byte3 are within their valid range, the byte4 outcome does not
depend on which valid byte2/byte3 values were chosen. Instead, byte2 and
byte3 are now held to a small hedge of representative valid prefixes
(range corners plus a midpoint) while byte4 is still swept exhaustively
over its full 0x00-0xFF range, since that is the actual property under
test. Also fixed the garbled "skip fourth second byte" comment in
unit-unicode3.cpp.

Verified offline: before the fix, the "wrong fourth byte" subcase
executes 0 assertions in all three files (proving it was dead code);
after the fix, it executes 11520 (unicode3), 34560 (unicode4), and 11520
(unicode5) assertions, and a deliberately reintroduced bug in the
lexer's byte4 range check causes it to fail (proving it is now
meaningful). Total per-file assertion counts grow by the same small
amounts, not by millions, and all other sections in these files still
pass unchanged.

Fixes #5416

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
The maintainer wants exhaustive coverage of every byte combination here
rather than the representative-prefix reduction, matching the style of
the sibling "wrong second/third byte" sections in the same files.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
@nlohmann nlohmann added the review needed It would be great if someone could review the proposed changes. label Sep 5, 2026
@github-actions github-actions Bot added L tests S and removed L labels Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review needed It would be great if someone could review the proposed changes. S tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unicode tests: "ill-formed: wrong fourth byte" sections never execute anything (byte3/byte4 typo)

1 participant