Skip to content

fix(libc): reject an invalid _WStoul base like Dinkumware instead of … - #10729

Merged
oneandonlydean merged 3 commits into
boykopovar:mainfrom
dwk001:fix/libc-wstoul-invalid-base
Nov 2, 2026
Merged

oneandonlydean merged 3 commits into
boykopovar:mainfrom
dwk001:fix/libc-wstoul-invalid-base

Conversation

@dwk001

@dwk001 dwk001 commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

What

_WStoul threw std::invalid_argument for a base other than 0 or 2 to 36. It now passes every base to wcstoul, which returns 0, stores the start of the string in the end pointer and leaves errno unchanged, as Dinkumware's xstoul.c does. Unverified on the console; noted in docs/dev/debt/libc.md.

Tested

Linux, full build with -DBUILD_TESTING=ON: ctest --test-dir build -R guest_wide_conversion passes with cases for bases 1, 37 and -2. tools/check_conventions.py --base origin/main reports one warning: the existing _WStoul/_Iswctype/swscanf debt entry was already over the 1000-character limit on main.

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

@github-actions github-actions Bot added the open PR limit Closed over the open PR limit: 1 without merged PRs, 20 with at least 1, none from 20 label Oct 24, 2026
@github-actions

Copy link
Copy Markdown

Thank you for your contribution! Authors with no pull requests merged into main can have 1 open pull request at a time, authors with at least 1 merged can have 20, and from 20 merged there is no limit. This PR has been automatically closed and labeled "open PR limit" because that limit was reached. This is not a judgment of the quality of your changes: reopen it once one of your open pull requests is merged or closed.

@github-actions github-actions Bot closed this Oct 24, 2026

@MarcZX10 MarcZX10 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.

Reviewed exact head 5d2b23c. Removing the _WStoul-only exception preserves the shared wcstoul parser behavior and the setError=false invalid-base path: zero result, end pointer at the original input, and unchanged errno. The tests cover bases 1, 37, and -2 with leading whitespace. I syntax-compiled the exact Strings.cpp and GuestWideConversion.cpp; conventions report 0 errors (one existing debt-length warning), and git diff --check passes. I could not run the full runtime test in this environment.

@github-actions github-actions Bot removed the open PR limit Closed over the open PR limit: 1 without merged PRs, 20 with at least 1, none from 20 label Oct 27, 2026
@oneandonlydean oneandonlydean self-assigned this Oct 27, 2026
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 27, 2026
@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

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.

Pull request Conflicting files (lines on main) Same exports Same new files
PR 9087 docs/dev/debt/libc.md L232
PR 9863 docs/dev/debt/libc.md L232
PR 12622 docs/dev/debt/libc.md L232

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.

Pull request Shared files not compared
PR 5021 core/libs/prx/libc/src/Strings.cpp
docs/dev/debt/libc.md
PR 7081 docs/dev/debt/libc.md
PR 7386 docs/dev/debt/libc.md
PR 7447 docs/dev/debt/libc.md
PR 7715 docs/dev/debt/libc.md
PR 8150 docs/dev/debt/libc.md
PR 8965 core/libs/prx/libc/src/Strings.cpp
docs/dev/debt/libc.md
PR 9340 docs/dev/debt/libc.md
PR 9371 docs/dev/debt/libc.md
PR 9447 docs/dev/debt/libc.md
PR 9526 docs/dev/debt/libc.md
PR 9929 docs/dev/debt/libc.md
PR 10405 docs/dev/debt/libc.md
PR 10987 docs/dev/debt/libc.md
PR 11332 docs/dev/debt/libc.md
PR 11625 docs/dev/debt/libc.md
PR 11831 docs/dev/debt/libc.md
PR 12560 docs/dev/debt/libc.md
PR 14212 docs/dev/debt/libc.md

@maskym

maskym commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Windows result for head 31cc00b5d8, merged into main 2ee8e9751a (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 #11172, #10780, #11614, #10631, #10582, #11547, #11584, #11572, all merged cleanly, so the run is a joint one; the tests of this PR are guest_wide_conversion. Together with the whole libc, CesCs, Font, FreeType, LibreSsl3 and NpUniversalDataSystem suites those tests belong to: 160/160 passed, this PR's in 0.04 s; the AudioOut suite separately 21/21. Not run on a PS5.

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

@JJenkx

JJenkx commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

@dwk001 a merge-order note from #12420 (swscanf: scanset ranges, string lengths and unknown conversions, measured on titles' libc.prx). The overlap check pairs it with this PR on the long swscanf entry in docs/dev/debt/libc.md (line 201). #12420 changes only that entry's "It throws for unknown length modifiers, scanset ranges, ..." sentence, so it's a text overlap, not a code one.

I'd suggest this PR land first. I'll rebase #12420 after it, so you have nothing to resolve. If you'd rather it go the other way, say so.

AI-assisted: yes (Claude Code).

@BunnyFl BunnyFl 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.

Approving head 31cc00b5d8. I read the diff and ran firmware 13.42's _WStoul (JhVR7D4Ax6Y): the decrypted system libSceLibcInternal.sprx's export, natively on a PC (not a console), with errno cleared and the end pointer pre-set before each call. Three identical runs.

  • " 1" with base 1, 37 and -2, "1" with base 1, "Z" with base 37 and " 1" with base 100 all return 0, leave the end pointer at the start of the string and leave errno at 0. That is what the new CheckStoul rows require.
  • Controls in the same run: " 1" base 10 gives 1 (end after 2 characters), " ff" base 16 gives 255, "0x1f" base 0 gives 31, "zz" base 36 gives 1295, and an overflowing decimal gives ULLONG_MAX with errno 34, so the end pointer and errno are both observed.

So the debt entry's "unverified on the console" can say that firmware 13.42's system module does the same, if you want to cite it.

@JJenkx

JJenkx commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

@dwk001 a merge-order question: a maintainer asked the authors of #12514 (swscanf floating fields as titles' libc.prx does) and of the PRs it overlaps to agree an order. The only overlap with this PR is in docs/dev/debt/libc.md: git merge-tree reports a textual conflict between the two PRs' edits to nearby debt entries. The code doesn't overlap.

Proposed order: this PR lands first, and #12514 rebases afterwards (#12514 will declare it in Depends on). Your PR needs no changes. Could you confirm that order works for you, or say if you'd rather #12514 went first?

AI-assisted: yes (Claude Code).

@dwk001

dwk001 commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

@dwk001 a merge-order question: a maintainer asked the authors of #12514 (swscanf floating fields as titles' libc.prx does) and of the PRs it overlaps to agree an order. The only overlap with this PR is in docs/dev/debt/libc.md: git merge-tree reports a textual conflict between the two PRs' edits to nearby debt entries. The code doesn't overlap.

Proposed order: this PR lands first, and #12514 rebases afterwards (#12514 will declare it in Depends on). Your PR needs no changes. Could you confirm that order works for you, or say if you'd rather #12514 went first?

AI-assisted: yes (Claude Code).

Confirmed, that order works for me: #10729 first, then #12514 rebases on top. Thanks for coordinating

# Conflicts:
#	docs/dev/debt/libc.md
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Nov 1, 2026
@oneandonlydean
oneandonlydean merged commit 307dd57 into boykopovar:main Nov 2, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants