Skip to content

fix(libSceNpCommon): keep the JSON text helpers inside their buffers and surrogate pairs - #12265

Merged
oneandonlydean merged 4 commits into
boykopovar:mainfrom
MarcZX10:fix/npcommon-json-text-bounds
Oct 28, 2026
Merged

oneandonlydean merged 4 commits into
boykopovar:mainfrom
MarcZX10:fix/npcommon-json-text-bounds

Conversation

@MarcZX10

Copy link
Copy Markdown
Contributor

What

Three defects in the JSON text helpers of libSceNpCommon/Strings.cpp. One commit each, then one test commit:

  • sceNpUtilJsonEscape appends with strcat, but its room check started the running length at 0 and ignored the text the buffer already holds, so escaping into a partly filled buffer wrote past its end: sceNpUtilJsonEscape(buf, 16, "\n\n\n", 3) on a buffer holding ten characters placed 16 characters and the terminator in 16 bytes. The running length now starts at the existing text length, and an already full destination returns NP_UTIL_ERROR_BUFFER_TOO_SMALL.
  • sceNpUtilJsonGetOneChar parsed the low half of a surrogate pair at offset 8 without checking that offsets 6 and 7 hold \u, so "\ud83dXXde00" decoded to U+1F600 and swallowed the two unrelated characters. sceNpUtilJsonUnescape already makes that check; a high surrogate not followed by \u now fails the same way an invalid low surrogate does.
  • sceNpUtilJsonUnescape applied its final count > dstSize check even for a null destination, so the size query (nullptr, 0, src, n, &length) returned BUFFER_TOO_SMALL and never wrote the length, although the per-character checks in the same function skip the comparison for a null destination. The check now applies only when there is a destination. The console's behaviour for this exact call is not measured; the change makes the size query consistent with the rest of the function.

The test commit adds the three assertions to TestJsonText in GuestNpCommonRuntime.cpp.

Tested

Linux x86-64, a throwaway archive build with the host CPU gate patched. guest_np_common_runtime passes. I did not run the new assertions against the unfixed code. tools/check_conventions.py --base upstream/main and git diff --check pass.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

…lJsonEscape

sceNpUtilJsonEscape appends to the destination with strcat, but the room check
started its running length at 0 and ignored the text the buffer already held,
so escaping into a partly filled buffer wrote past its end:
sceNpUtilJsonEscape(buf, 16, "\n\n\n", 3) on a buffer holding ten characters
placed 16 characters and the terminator in 16 bytes.

The running length now starts at the length of the existing text, and a
destination that is already full returns NP_UTIL_ERROR_BUFFER_TOO_SMALL.
…NpUtilJsonGetOneChar

After a high surrogate, sceNpUtilJsonGetOneChar parsed four hex digits at offset
8 without checking that offsets 6 and 7 hold a backslash and a 'u'. The string
"\ud83dXXde00" decoded to U+1F600 and consumed 12 bytes, swallowing the two
unrelated characters. sceNpUtilJsonUnescape already makes that check. A high
surrogate not followed by \u now takes the same failure path as an invalid low
surrogate.
…ll destination

The final room check ran even when dst was null, so a size query such as
(nullptr, 0, src, n, &length) returned NP_UTIL_ERROR_BUFFER_TOO_SMALL and never
wrote the required length, although the per-character checks in the same
function already skip the comparison for a null destination. The check now
applies only when there is a destination.
…ation query, split a surrogate pair

sceNpUtilJsonEscape has to stay inside a partly filled 16 byte buffer and refuse
one without a terminator, sceNpUtilJsonUnescape has to report the length of a
query with a null destination and size 0, and sceNpUtilJsonGetOneChar has to
reject a high surrogate that is not followed by a second unicode escape.
@JJenkx

JJenkx commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

A Windows run, since the description lists none. Head e6083ad5c5 was merged onto main 0cd07dc4ab with no conflicts. Setup: Windows 11 Enterprise 10.0.22621, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release, BUILD_TESTING=ON.

libs builds, and guest_np_common_runtime, including this PR's GuestNpCommonRuntime.cpp checks, passes in each of 3 runs.

AI-assisted: yes (Claude Code).

@B-Howell

Copy link
Copy Markdown
Contributor

Windows result in the project's real build, with the control the description lists as not run ("I did not run the new assertions against the unfixed code").

Setup: Windows 11 Pro 26300, AMD Ryzen 7 7800X3D, WinLibs GCC 15.2.0 r7, Ninja Release, BUILD_TESTING=ON, libs rebuilt. Head e6083ad5c5e9e790b4e8bd45771491440d541cd7 (70 behind main) merged onto main 0cd07dc4ab25d924cbcebee5e0468a1b785042f7 (clean).

Test: ctest -R '^guest_np_common_runtime$' --repeat until-fail:10

  • PR merged onto main: passes all 10 runs (0.7 s total).
  • Control: libSceNpCommon/Strings.cpp restored from main, test kept: aborts (0xc0000409, FATAL: abort() (SIGABRT)) on the first run. The test prints no failing line, so I can't say which of the new buffer or surrogate checks trips first.

Not tested: Linux in a real build. My WSL tree (GCC 11.4) stops on an unrelated main issue in that target's dependencies: core/shader/recompiler/ShaderDiskCache.cpp:90: static assertion failed: RecompileResult changed. It fails the same way with Strings.cpp from main, so it says nothing about this PR. Also not tested: macOS, any title.

AI-assisted (Claude Code).

@maskym

maskym commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Windows result for head e6083ad5c5, merged into main 87b810a1db (the PR's Tested section names no Windows run).

Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, BUILD_TESTING=ON, Ninja. Build clean.

The tree also carried the open PRs #12290, #12288, #12285, #12269, #12232, #12206, #12201, #12199, #12197, #12165, #12146, all merged cleanly, so the run is a joint one; the tests of this PR are guest_np_common_runtime (0.08 s), all passed (every test registered from the tree's changed test files: 38/38). Not run on a PS5.

AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants