Skip to content

Fix NUL bytes in UBJSON/BJData high-precision numbers - #5760

Merged
nlohmann merged 6 commits into
nlohmann:developfrom
fhgffy:fix/ubjson-high-precision-nul-5753
Oct 6, 2026
Merged

nlohmann merged 6 commits into
nlohmann:developfrom
fhgffy:fix/ubjson-high-precision-nul-5753

Conversation

@fhgffy

@fhgffy fhgffy commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Reject NUL bytes inside length-delimited UBJSON/BJData high-precision (H) number payloads. H i 3 1 NUL x used to be accepted as the unsigned integer 1, because the lexer stops at the NUL and never sees the rest of the payload. It now produces parse_error.115.

The payload read loop returns the error as soon as it reads a NUL, so the byte offset points at the NUL and the payload is not lexed. The message follows the other binary reader errors: invalid number text; last byte: 0x00. A payload that is cut off after a NUL reports the NUL rather than end of input.

Regression cases in both format suites cover hidden bytes after a NUL, a trailing NUL, a truncated payload containing a NUL, a nested array, non-throwing parsing, and a valid number. single_include/nlohmann/json.hpp carries the same change.

Fixes #5753.

Validation on Windows x86_64, MinGW GCC 15.1.0, C++11:

  • The new cases fail without the fix, because the malformed payloads are accepted.

  • The UBJSON and BJData suites pass with both include/ and single_include/, and with JSON_STRICT_NUL_HANDLING=1.

  • Coverage was not measured locally.

  • The changes are described in detail, both the what and why.

  • An existing issue is referenced.

  • Code coverage remained at 100% (not measured locally; awaiting CI).

  • Source code is amalgamated.

OSS-Fuzz and documentation checklist items are not applicable to this change.

@fhgffy
fhgffy requested a review from nlohmann as a code owner October 4, 2026 17:14
exception_message(concat("invalid number text: ", number_lexer.get_token_string()), "high-precision number"), nullptr));
}

// 2026-10-05: A NUL must not terminate a length-delimited number payload; preserve the lexer diagnostic.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better performance-wise to catch this before pushing it into number_vector?

        for (std::size_t i = 0; i < size; ++i)
        {
            get();
            if (JSON_HEDLEY_UNLIKELY(!unexpect_eof("number")))
            {
                return false;
            }
            if (JSON_HEDLEY_UNLIKELY(current == '\0')
            {
              return sax->parse_error(chars_read, ...
            }
            number_vector.push_back(static_cast<char>(current));
        }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved NUL detection into the read loop in e9b4643, so there's no separate payload scan. I kept error reporting after the full read to preserve the byte offset and lexer message; truncated payloads still report EOF (110). Added a truncated-NUL case to both suites. The UBJSON/BJData tests pass with both NUL macro settings.

// 2026-10-05:长度限定的数字载荷不能把 NUL 当作真实结尾,沿用词法器的错误文本。
if (JSON_HEDLEY_UNLIKELY(std::find(number_vector.begin(), number_vector.end(), '\0') != number_vector.end()))
// Reject NULs after a complete read to preserve EOF errors and lexer diagnostics.
if (JSON_HEDLEY_UNLIKELY(contains_nul))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept error reporting after the full read to preserve the byte offset and lexer message; truncated payloads still report EOF (110). Added a truncated-NUL case to both suites. The UBJSON/BJData tests pass with both NUL macro settings.

It's up to @nlohmann, but I don't see the utility in preserving the lexer message. It causes extra work before reporting the error, and could change the error from NUL to EOF. I think the byte offset should be the exact location of the NUL byte.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. In fecb084 the read loop returns the parse error as soon as it reads the NUL, so the byte offset is the NUL's position and the payload is no longer lexed first. The message follows the other binary reader errors: invalid number text; last byte: 0x00. A payload cut off after a NUL now reports that NUL (115) instead of end of input. The tests cover that case alongside the trailing and nested ones.

The UBJSON and BJData tests pass with both the split and amalgamated headers, and with JSON_STRICT_NUL_HANDLING=1. The previous commit was missing its sign-off, which is now added.

@gregmarr

gregmarr commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Your most recent commit did not contain the DCO signoff.

@fhgffy
fhgffy force-pushed the fix/ubjson-high-precision-nul-5753 branch from a050f87 to fecb084 Compare October 4, 2026 19:43

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann added this to the Release 3.13.0 milestone Oct 4, 2026
@nlohmann nlohmann added the 🚀 ready to merge Ready to merge - just waiting for CI to complete. label Oct 4, 2026
@github-actions github-actions Bot added aspect: binary formats BSON, CBOR, MessagePack, UBJSON M tests labels Oct 5, 2026
@fhgffy

fhgffy commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

The GCC/C++26 failure is the existing CBOR narrowing warning at binary_reader.hpp:668, already covered by #5764. I checked that get_cbor_negative_integer() is unchanged from this PR's base (4f69be8). The other five GCC standard jobs were cancelled by fail-fast. I'll keep this PR focused on the NUL fix and pick up the develop fix if a branch update is needed.

@nlohmann

nlohmann commented Oct 5, 2026

Copy link
Copy Markdown
Owner

Sorry about the CI noise. PR #5764 hopefully fixes this. I'll check back on this PR once #5764 is green.

@fhgffy

fhgffy commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

The sanitizer job has a separate failure: 137/138 tests passed, with test-std-format_cpp20 stopping at libstdc++ 14's format:3906 when -1 is converted to size_t. This matches GCC bug 119429, whose reproducer uses only std::print. I checked that unit-std-format.cpp and the sanitizer flags are unchanged from this PR's base. It may need separate toolchain handling from the CBOR and documentation fixes in #5764.

@nlohmann

nlohmann commented Oct 5, 2026

Copy link
Copy Markdown
Owner

Thanks for tracking this down! Your analysis is right: libstdc++ 14's <format> has _Scanner(basic_string_view<_CharT>, size_t __nargs = -1), so with -fsanitize=integer, every std::format call fails as implicit-integer-sign-change (GCC bug 119429). develop fails the same way, so this has nothing to do with your change.

#5764 now also fixes this. It adds cmake/clang_sanitizer_ignorelist.txt, which ci_test_clang_sanitizer passes via -fsanitize-ignorelist. The list excludes only that check, and only for libstdc++'s <format> header, so sign changes in the library and the tests are still reported. Once #5764 is merged, updating this branch from develop should make the sanitizer job pass.

(This comment was written by Claude Code on my behalf.)

{
auto last_token = get_token_string();
return sax->parse_error(chars_read, last_token, parse_error::create(115, chars_read,
exception_message(concat("invalid number text; last byte: 0x", last_token), "high-precision number"), nullptr));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know that the last byte is 0 because that's why you're throwing the error. No need to get it and convert it to a string.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in f2c82f7: the SAX token is now "00" and the error text is fixed, so this branch no longer calls get_token_string() or concat(). The error message and byte offsets are unchanged. The UBJSON/BJData suites pass with split and single headers, with both NUL macro settings.

@gregmarr gregmarr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nit for some unnecessary work in an error path, but otherwise looks good.

@nlohmann

nlohmann commented Oct 6, 2026

Copy link
Copy Markdown
Owner

The develop branch is green now. Please rebase a last time and we're should be good to go!

@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Oct 6, 2026
fhgffy added 6 commits October 6, 2026 14:15
Signed-off-by: fhgffy <102001626+fhgffy@users.noreply.github.com>
Signed-off-by: fhgffy <102001626+fhgffy@users.noreply.github.com>
Signed-off-by: fhgffy <102001626+fhgffy@users.noreply.github.com>
Signed-off-by: fhgffy <102001626+fhgffy@users.noreply.github.com>
Return the parse error from the read loop so the byte offset points at the NUL, and drop the separate check after lexing.

Signed-off-by: fhgffy <102001626+fhgffy@users.noreply.github.com>
2026-10-06: Use the known zero byte directly instead of formatting and concatenating it. Preserve the SAX token, error message, and byte offset.
Signed-off-by: fhgffy <102001626+fhgffy@users.noreply.github.com>
@fhgffy
fhgffy force-pushed the fix/ubjson-high-precision-nul-5753 branch from f2c82f7 to bdb821a Compare October 6, 2026 06:18
@fhgffy

fhgffy commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto develop at 0490778 and pushed bdb821a. The UBJSON/BJData suites pass locally with split and single headers, with both NUL settings. I also reran the allocator and regression2 suites; both pass. The new GitHub Actions runs are waiting for approval.

@nlohmann nlohmann removed the please rebase Please rebase your branch to origin/develop label Oct 6, 2026
@fhgffy

fhgffy commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

The rbegin() failures are covered by #5767. There is also a separate -Werror=switch-enum failure in has_no_children() at json.hpp:630: the switch only explicitly handles the array and object enumerators. That switch is still unchanged in #5767.

I reproduced this with MinGW GCC 15.1/C++11 on unmodified sources from develop at 0490778; the same syntax-only compile passes at 5379e04, before #5762:

g++ -std=c++11 -Wswitch-enum -Werror=switch-enum -Iinclude -fsyntax-only tests/abi/diag/diag_off.cpp

The split json.hpp and diag_off.cpp are identical between this PR and its base, so this also needs a develop fix.

@fhgffy

fhgffy commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Opened #5770 for the remaining switch-enum failure. It adds the missing enum labels to the existing branch and synchronizes the single header. The ABI diagnostic builds now pass locally with -Wswitch-enum -Werror=switch-enum in both header modes.

@nlohmann
nlohmann merged commit 69874e4 into nlohmann:develop Oct 6, 2026
22 of 169 checks passed
@nlohmann

nlohmann commented Oct 6, 2026

Copy link
Copy Markdown
Owner

Thanks!

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

Labels

aspect: binary formats BSON, CBOR, MessagePack, UBJSON M 🚀 ready to merge Ready to merge - just waiting for CI to complete. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UBJSON/BJData high-precision numbers accept a NUL-terminated prefix inside their payload

3 participants