Repository navigation
fix(libSceIcu): follow ICU for null escape inputs and negative u_unescape capacities - #12451
Conversation
…cape capacities ICU 78.1's u_unescapeAt dereferences the offset and calls the callback without checks, and u_unescape reads the source without one, so a null one faults: throw std::invalid_argument there instead of NotImplemented_nid_no_patch. A negative u_unescape capacity is clamped by _appendUChars and fails every room check, so nothing is written and the full length is returned; only a capacity whose signed difference with the output length overflows, which ICU leaves undefined, still throws.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 0788324. The ICU behavior claims match the cited implementation: null pointers that ICU dereferences now use the project invalid-argument exception policy, negative destination capacity writes nothing, and cases where ICU’s signed capacity subtraction would overflow remain explicit throws. The tests cover null precedence, unchanged destination bytes, and boundary cases around INT32_MIN. Hosted conventions, intake, eligibility, report, labels and applicable Windows validation pass; platform-specific jobs outside this change are skipped. I have not independently run the Windows test suite, and no console ABI result is claimed. The current-main merge is clean and git diff --check passes.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
Windows run of this head (
AI-assisted: yes (Claude Code). |
|
Checked the null-input and negative-capacity paths against ICU 78.1's |
What
u_unescapeAtandu_unescapefollow ICU 78.1'sustring.cpp, but throwNotImplemented_nid_no_patchfor null pointers and for a negative capacity with a destination, sotools/progress.pycounts both as stubs. ICU answers or faults in each of these cases:u_unescapeAtreads*offsetfirst and callscharAtonce the offset is in range, checking neither. A null offset, or a null callback with an in-range offset, now throwsstd::invalid_argument(APS5_INVALID_ARG_EX), as fix(libSceLibreSsl): reject null strings in the ASN.1 accessors as invalid arguments #11111 did where LibreSSL faults. An out-of-range offset still returns -1 without calling the callback.u_unescapereads*srcwithout a check, so a null source now throwsstd::invalid_argument._appendUCharsclamps the capacity to 0, andU16_LENGTH(c32) <= (destCapacity - i),i < destCapacityanddestCapacity > 0are all false. Nothing is written and the full length is returned, as for a capacity of 0. The existing code already gives that answer once the throw is gone.destCapacity - iinint32_tat each escape and before each escape-free segment it appends. For a capacity close toINT32_MINthat subtraction overflows, which is undefined. That case still throwsNotImplemented_nid_no_patch, at the points where ICU computes it.A source longer than
INT32_MAXbytes also still throws: ICU truncates the remaining length it passes tou_unescapeAt, and its output count overflows. Sou_unescapestays counted as to do. The debt entry now says which cases throw which exception.tools/progress.py --module libSceIcu: 11 → 12 implemented (u_unescapeAt), 109 → 108 to do.Tested
Windows 11 Pro 10.0.26200, WinLibs GCC 15.2.0 r7 (x86_64-ucrt-posix-seh), Release,
BUILD_TESTING=ON. Written and tested onmain750cf6cf48, then rebased ontod4cb89300f, which changes none of these files; the tests below andcheck_conventionswere rebuilt and rerun on the rebased head, while the control andpr_overlapran before the rebase.guest_icu_unescape:u_unescapeAtnow requirestd::invalid_argumentinstead of any exception, and both null at once are covered.std::invalid_argumentand leaves the destination untouched.INT32_MIN + 1,INT32_MIN) with escapes, plain text, a failing escape and an empty string return the full length (0 for the failing escape). The destination and the units around it stay untouched."a\\n","\\na"and"\\nab"withINT32_MIN, and"ab\\n"withINT32_MIN + 1. One unit more of capacity returns the length ("ab\\n"withINT32_MIN + 2,"\\nab"withINT32_MIN + 1).Pass, 10 runs (
ctest --repeat until-fail:10).Control: with
main'sUnescape.cppand this test,guest_icu_unescapefails withu_unescapeAt: null callback not implemented(exit0xc0000409).guest_icu_cstringandguest_icu_version: pass.public_headersandpublic_headers_compile_*: 65/65 pass.python tools/check_conventions.py --base origin/mainwith this title and body: all checks pass.python tools/pr_overlap.py --branch fix/icu-unescape-invalid-inputfinds no overlap. It could not comparedocs/dev/debt/libSceIcu.mdwith feat(libSceIcu): implement the numeric platform helpers #11088, feat(libSceIcu): implement u_getVersion and u_errorName #11115, feat(libSceIcu): implement the constrained field position exports #11118, feat(libSceIcu): implement the code point set exports #11122, feat(libSceIcu): implement the enumeration dispatch exports #11132, feat(libSceIcu): implement the string and UTF-8 character iterators #11136 and feat(libSceIcu): implement UTF-32, Java UTF-8, comparison and configuration exports #12320, which already conflict withmainthere.git merge-treeof this head with each of them keeps the Escape decoding entry outside every conflict block. feat(libSceIcu): implement UTF-16 set searches and tokenization #10962 to feat(libSceIcu): implement UTF-16 search and memory primitives #10967 merge cleanly.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)