Skip to content

Reject len == SIZE_MAX in unsafe_yyjson_mut_str_alc#268

Merged
ibireme merged 1 commit into
ibireme:masterfrom
rootvector2:fix-mut-str-alc-len-overflow
May 23, 2026
Merged

Reject len == SIZE_MAX in unsafe_yyjson_mut_str_alc#268
ibireme merged 1 commit into
ibireme:masterfrom
rootvector2:fix-mut-str-alc-len-overflow

Conversation

@rootvector2

Copy link
Copy Markdown
Contributor

Bug

unsafe_yyjson_mut_str_alc() in src/yyjson.h computes len + 1 (to reserve a
null terminator) without first checking whether the caller-supplied len is
(size_t)-1. When that happens:

  • len + 1 wraps to 0.
  • The grow path passes 0 to unsafe_yyjson_str_pool_grow(), which checks
    len > USIZE_MAX - sizeof(yyjson_str_chunk) -- 0 > max_len is false, so the
    grow proceeds and allocates a tiny chunk.
  • The non-grow path is also bypassed: (pool->end - pool->cur) <= len is true
    for the wrapped SIZE_MAX value.
  • pool->cur = mem + len + 1 wraps back to mem.
  • unsafe_yyjson_mut_strncpy() (and every caller listed below) then runs
    memcpy(mem, str, (size_t)-1), which reads SIZE_MAX bytes from the caller's
    buffer into a chunk that does not have anywhere close to that much space.

The vulnerable helper sits behind every public mutable-string API that forwards
an untrusted length unchanged, including yyjson_mut_strncpy(),
yyjson_mut_rawncpy(), yyjson_mut_obj_add_strncpy(),
yyjson_mut_arr_with_strncpy(), and yyjson_mut_arr_with_strn() (the last via
unsafe_yyjson_set_strn after copy). Any service that derives the length from
attacker-controlled data and passes it straight in is exposed.

Reproducer

Compiled against the current tree with AddressSanitizer:

yyjson_mut_doc *doc = yyjson_mut_doc_new(NULL);
const char src[8] = "abcdefg";
yyjson_mut_strncpy(doc, src, (size_t)-1);
==ERROR: AddressSanitizer: negative-size-param: (size=-1)
    #0 __asan_memcpy
    #1 main repro_strncpy.c:28        <-- inside unsafe_yyjson_mut_strncpy

Fix

Reject len == (size_t)-1 at the top of unsafe_yyjson_mut_str_alc() so that
len + 1 is always well-defined for the rest of the function. After the fix
the reproducer returns NULL (the documented error path for the public APIs)
instead of corrupting memory.

The pre-existing bound check in unsafe_yyjson_str_pool_grow()
(len > USIZE_MAX - sizeof(yyjson_str_chunk)) already rejects every other
oversize value cleanly; only the exact SIZE_MAX wrap-around needed an
additional guard.

Test plan

  • Configured with -DYYJSON_BUILD_TESTS=ON -DYYJSON_SANITIZER=undefined on
    Apple clang and rebuilt.
  • ctest -- 12/12 tests pass.
  • Reran the reproducer above; yyjson_mut_strncpy now returns NULL instead
    of triggering an ASan report.

Change is one file, four added lines (one branch + a short comment).

unsafe_yyjson_mut_str_alc() in src/yyjson.h computes `len + 1` (to
reserve a trailing null byte) without checking whether the input
`len` is `(size_t)-1`. When that happens:

  * `len + 1` wraps to 0.
  * The grow path passes 0 to unsafe_yyjson_str_pool_grow(), which
    allocates a tiny chunk and returns success.
  * The non-grow path is also bypassed because the
    `(pool->end - pool->cur) <= len` test compares against the wrapped
    SIZE_MAX value.
  * `pool->cur = mem + len + 1` wraps to `mem`.
  * unsafe_yyjson_mut_strncpy() then runs
    `memcpy(mem, str, (size_t)-1)`, which reads SIZE_MAX bytes from
    the caller's buffer into a chunk that does not have anywhere near
    that much space.

This affects every public mutable-string API that forwards an
untrusted length unchanged: yyjson_mut_strncpy(), yyjson_mut_rawncpy(),
yyjson_mut_obj_add_strncpy(), yyjson_mut_arr_with_strncpy(),
yyjson_mut_arr_with_strn(), and any caller of unsafe_yyjson_mut_strncpy.

Reproducer (ASan, on this tree before the fix):

    yyjson_mut_doc *doc = yyjson_mut_doc_new(NULL);
    const char src[8] = "abcdefg";
    yyjson_mut_strncpy(doc, src, (size_t)-1);

    ==ERROR: AddressSanitizer: negative-size-param: (size=-1)
        #0 __asan_memcpy
        ibireme#1 main repro.c:...    -- inside unsafe_yyjson_mut_strncpy

Fix: reject `len == (size_t)-1` at the top of the allocator helper, so
`len + 1` is always well-defined. After the fix the same call returns
NULL (the documented error behavior) instead of corrupting memory. The
existing bound in unsafe_yyjson_str_pool_grow() already handles every
other large value, so no other changes are needed.

Tests: configured with -DYYJSON_BUILD_TESTS=ON -DYYJSON_SANITIZER=undefined
on Apple clang and ran ctest -- 12/12 pass.
@codecov

codecov Bot commented May 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.49%. Comparing base (95f4c61) to head (6945ccb).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #268   +/-   ##
=======================================
  Coverage   98.49%   98.49%           
=======================================
  Files           2        2           
  Lines        7699     7700    +1     
=======================================
+ Hits         7583     7584    +1     
  Misses        116      116           
Flag Coverage Δ
unittests 98.49% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ibireme
ibireme merged commit 99cc435 into ibireme:master May 23, 2026
46 checks passed
@ibireme

ibireme commented May 23, 2026

Copy link
Copy Markdown
Owner

Thanks for the patch, merged.

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.

2 participants