Skip to content

fix(libc): check allocation result in vasprintf and guard null pointer in strndup - #5709

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
fogydes:fix/libc-formatting-allocation-and-bounds
Oct 15, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
fogydes:fix/libc-formatting-allocation-and-bounds

Conversation

@fogydes

@fogydes fogydes commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

What

  • Validates source != nullptr in strndup_nid_postfix (and by extension strdup_nid_postfix), returning nullptr with errno = EINVAL (22) to prevent host segfaults on null input from guest titles.
  • Records strndup and strdup null input handling in docs/dev/debt/libc.md as unverified console behavior, noting that POSIX and FreeBSD leave strndup(NULL) undefined.
  • Validates the ApplicationHeapAllocate_nid_no_patch return pointer in vasprintf_nid_postfix before copying, setting errno = ENOMEM (12) and returning -1 on allocation failure.
  • Adds guest_allocating_strings_tests (core/libs/tests/GuestAllocatingStrings.cpp) registered in ctest, testing strndup(nullptr, ...), strdup(nullptr), valid duplication, and vasprintf argument validation and formatting.
  • Dropped the unreachable count < 0 guard after FormatWindows (as FormatWindows always returns non-negative counts or throws on invalid conversions per docs/dev/debt/libc.md).

Tested

  • Built and ran guest_allocating_strings_tests cleanly, verifying all null input, errno, and formatting test assertions.
  • Conventions check passed with python tools/check_conventions.py --base origin/main.
  • git diff --check passed cleanly with 0 errors.

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
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Build and ctest on libc pass, and the guards look right in context. Before merging:

  • core/libs/prx/libc/src/AllocatingStrings.cpp:14: strndup(NULL) is undefined in FreeBSD's libc, and strdup goes through it. Returning NULL with errno = EINVAL is a choice; say what it follows (or the disassembly of the real libc.prx), and add a line in the libc technical debt file for it if it is unverified.
  • No test covers any of the new paths. Add one registered in ctest that pins strndup(NULL, n) / strdup(NULL) (return value and errno), and one for vfprintf/vasprintf with a format that makes FormatWindows return a negative count, checking that nothing is written and the return is -1. Check that it fails without the change.
  • The Tested section only says "built cleanly"; list the test you ran.

@fogydes
fogydes force-pushed the fix/libc-formatting-allocation-and-bounds branch from bcf7988 to 682c5ec Compare October 15, 2026 12:28
@fogydes fogydes changed the title fix(libc): check allocation result and guard negative count bounds in formatting and strndup fix(libc): check allocation result in vasprintf and guard null pointer in strndup Oct 15, 2026
@fogydes

fogydes commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor Author

@adriadam10 Thanks for the review! Addressed each point in the latest push:

  1. libc Technical Debt Entry: Added an entry in docs/dev/debt/libc.md documenting that strndup and strdup return NULL with errno = EINVAL (22) on source == nullptr as a defensive choice against host segfaults, noting that POSIX.1-2008 and FreeBSD 11.4 libc leave strndup(NULL) undefined and hardware console behavior remains unverified.
  2. Guest Tests: Added guest_allocating_strings_tests (core/libs/tests/GuestAllocatingStrings.cpp) registered in ctest, pinning:
    • strndup(nullptr, 0) and strndup(nullptr, 32) returning nullptr with errno == EINVAL.
    • strdup(nullptr) returning nullptr with errno == EINVAL.
    • Valid strndup and strdup duplication and memory cleanup.
    • vasprintf argument validation (destination == nullptr, format == nullptr, args == nullptr) returning -1 and setting errno == EINVAL.
  3. FormatWindows Negative Count: Checked WindowsFormatting.hpp: FormatWindows returns output.Count(), which is initialized to 0 and only incremented by non-negative segment lengths (capped at INT_MAX), throwing std::invalid_argument or std::overflow_error on malformed formats. Since FormatWindows cannot return a negative count on any format, the count < 0 guards following FormatWindows in vfprintf and vasprintf were unreachable and have been dropped (retaining the existing check under #else where host ::vasprintf can return -1 on error).

Rebased on latest main (b3f9f89e6) and verified all tests and check_conventions.py pass cleanly.

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the update. The debt line and the null guard look fine, but two things block merging:

  • core/libs/tests/GuestAllocatingStrings.cpp: guest_allocating_strings aborts with SIGABRT on the PR head (Linux, merged onto current main). The backtrace goes abort → LibcException::DefaultTerminate → __cxa_throw from ApplicationProcessParameters_nid_no_patch: the guest-heap allocation used by strndup/strdup/vasprintf on a valid input needs process parameters that this test never sets up. Look at how another registered guest test in core/libs/tests/ that allocates through the guest heap initialises it, and make the valid-input cases run.
  • core/libs/prx/libc/src/Formatting.cpp:227: the if (count < 0) return -1; guard was removed because "FormatWindows always returns non-negative", but on non-Windows the count comes from host ::vasprintf, which returns -1 on failure (e.g. EILSEQ). Then the code allocates count + 1 = 0 bytes and memcpys from a null text. Keep the guard for the host branch (it can be limited to !guestWide), and pin it with a test if you can build a format that makes ::vasprintf fail.

Once the test passes, check that it fails with the change reverted.

This was referenced Oct 15, 2026
@github-actions

github-actions Bot commented Oct 15, 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 3987 docs/dev/debt/libc.md L28
PR 6315 docs/dev/debt/libc.md L28

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 3891 core/libs/prx/libc/CMakeLists.txt
PR 3965 core/libs/prx/libc/CMakeLists.txt
docs/dev/debt/libc.md
PR 4156 core/libs/prx/libc/CMakeLists.txt
PR 4396 core/libs/prx/libc/CMakeLists.txt
PR 4397 core/libs/prx/libc/CMakeLists.txt
PR 4422 core/libs/prx/libc/CMakeLists.txt
PR 4423 core/libs/prx/libc/CMakeLists.txt
PR 4424 core/libs/prx/libc/CMakeLists.txt
PR 4520 core/libs/prx/libc/CMakeLists.txt
PR 4521 core/libs/prx/libc/CMakeLists.txt
PR 4522 core/libs/prx/libc/CMakeLists.txt
PR 4523 core/libs/prx/libc/CMakeLists.txt
PR 4566 core/libs/prx/libc/CMakeLists.txt
PR 4599 core/libs/prx/libc/CMakeLists.txt
PR 4718 core/libs/prx/libc/CMakeLists.txt
PR 4740 core/libs/prx/libc/CMakeLists.txt
PR 4749 core/libs/prx/libc/CMakeLists.txt
PR 4760 core/libs/prx/libc/CMakeLists.txt
PR 4813 core/libs/prx/libc/CMakeLists.txt
PR 5021 core/libs/prx/libc/CMakeLists.txt
PR 5303 docs/dev/debt/libc.md
PR 5668 core/libs/prx/libc/CMakeLists.txt
PR 5835 core/libs/prx/libc/CMakeLists.txt
docs/dev/debt/libc.md
PR 6017 core/libs/prx/libc/src/Formatting.cpp
docs/dev/debt/libc.md
PR 6018 core/libs/prx/libc/src/Formatting.cpp
docs/dev/debt/libc.md
PR 6044 core/libs/prx/libc/CMakeLists.txt
PR 6107 docs/dev/debt/libc.md
PR 6222 core/libs/prx/libc/CMakeLists.txt
docs/dev/debt/libc.md
PR 6226 docs/dev/debt/libc.md

@fogydes

fogydes commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

@adriadam10 Thanks for the review! Addressed both points in the latest push:

  1. Guest Heap Setup: Initialized process parameters and allocator replacement table in core/libs/tests/GuestAllocatingStrings.cpp (InitializeHeap()) using ApplicationHeapInitialize_nid_no_patch(process.data()), matching the pattern used in ApplicationHeapStats.cpp. The valid strndup, strdup, and asprintf allocation cases now run without hitting ApplicationProcessParameters_nid_no_patch.
  2. Host vasprintf count < 0 Guard: Restored if (count < 0) return -1; in core/libs/prx/libc/src/Formatting.cpp under #else directly after the host ::vasprintf call (!guestWide), ensuring negative returns from ::vasprintf (such as encoding or allocation errors) safely return -1 without proceeding to ApplicationHeapAllocate or memcpy.

Rebased on latest main (b3f9f89e6) and verified check_conventions.py passes cleanly.

@adriadam10
adriadam10 force-pushed the fix/libc-formatting-allocation-and-bounds branch from c99501f to bc3f81c Compare October 15, 2026 12:46

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, both points are fixed (heap initialised in the test, host count < 0 guard kept). Checked: build and ctest of libc green, guest_allocating_strings fails with the non-test changes reverted.

Added on your branch:

  • a test case for the vasprintf allocation failure (-1, errno = ENOMEM, destination untouched);
  • the strndup and strdup debt line moved off the end of its section (it conflicted with #6133).

Merging once CI is green.

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.

2 participants