Skip to content

fix(libc): return the fatal unwind error for an unknown personality on Windows - #9585

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
DavidSamir:fix/win-unknown-personality
Oct 26, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
DavidSamir:fix/win-unknown-personality

Conversation

@DavidSamir

@DavidSamir DavidSamir commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

What

On Windows, LibcUnwind::CallPersonality checks whether the personality address is a jmp [rip+disp] import thunk, and follows it, before comparing it to the known personalities. It does that by reading the bytes at the address. An unknown personality that isn't mapped therefore crashed the process with an access violation. On Linux the same input returns _URC_FATAL_PHASE1_ERROR / _URC_FATAL_PHASE2_ERROR.

exception_personality covers exactly this case (CallPersonality(0x1000, ...), ExceptionPersonality.cpp:87-88), but it was registered only under if(UNIX). Built on Windows, it segfaulted at that line, and every earlier check passed.

How:

Tested

  • Windows 11 Pro 26200, WinLibs GCC 15.2.0, Release. CPU-side only, no GPU involved. exception_personality passes.
  • Negative control: with the Unwind.cpp change reverted, it segfaults.
  • The 31 exception, unwind, backtrace, overflow_error and bad_optional_access tests pass.
  • tools/check_conventions.py passes.

Overlap (tools/pr_overlap.py --branch plus git merge-tree):

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 commented Oct 22, 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 10927 core/libs/prx/libc/CMakeLists.txt L942

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/CMakeLists.txt
PR 6713 core/libs/prx/libc/CMakeLists.txt
PR 7081 core/libs/prx/libc/CMakeLists.txt
PR 7201 core/libs/prx/libc/CMakeLists.txt
core/libs/prx/libc/src/exception/Unwind.cpp
PR 7447 core/libs/prx/libc/CMakeLists.txt
PR 7715 core/libs/prx/libc/CMakeLists.txt
PR 7851 core/libs/prx/libc/CMakeLists.txt
PR 8150 core/libs/prx/libc/CMakeLists.txt
PR 9772 core/libs/prx/libc/CMakeLists.txt
PR 9929 core/libs/prx/libc/CMakeLists.txt

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

The Windows guard addresses the reported invalid-address crash, and the Windows-only test registration is appropriately separate from the existing UNIX registration. The rerun conventions check on the current head passes, so the initial failing run is superseded.

The remaining required coordination is with #8087. It is open and changes Unwind.cpp, the same file this PR modifies. The description already gives a concrete composition plan if #8087 lands first, but the checklist still says Depends on: none, which does not record the required order. Please agree the order with #8087's author and update Depends on (or document the agreed reverse order and corresponding rebase). The repository rule requires overlap order to be agreed and listed in the checklist.

Hosted relinker, ASan, register-context and write-watch checks pass; the Linux and Windows product build jobs are skipped. The Windows test results in the description are author-reported; I did not run tests locally.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 22, 2026
@DavidSamir

Copy link
Copy Markdown
Contributor Author

Merged main after #8087 landed, in a new merge commit with no force-push. As this PR's description said it would, the two Readable checks now live in #8087's ResolveImport(Word): the thunk bytes and the slot are read only when VirtualQuery reports them readable, and otherwise the address comes back unchanged. That covers both CallPersonality and VirtualUnwind's .sehgcc handler lookups. The diff against main is now just Readable, the guarded ResolveImport, and the exception_personality registration.

Re-run on the merge (Windows 11, WinLibs GCC 15.2):

  • exception_personality passes, and the 31 exception, unwind, backtrace, overflow_error and bad_optional_access tests pass.
  • Negative control: with the two checks removed from ResolveImport, exception_personality segfaults again.

AI-assisted: yes

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 22, 2026
@fogydes

fogydes commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Reviewed; verified the guard and the test wiring. On main, ResolveImport unconditionally read code[0]/code[1] and then the IAT slot at address + 6 + displacement, so a personality pointer into anything unmapped faulted inside the unwinder itself. The branch now gates both reads with a VirtualQuery-based Readable check; when the reads fail the address is returned untouched, the two known-personality comparisons (dllimport/bound personalities) fall through, and the search/phase-2 callers return _URC_FATAL_PHASE1_ERROR/_URC_FATAL_PHASE2_ERROR — the Linux behavior, and both callers propagate non-CONTINUE_UNWIND results so nothing swallows it. The diff leaves every mapped ff 25 thunk byte-identical to main (only unmapped inputs change behavior), and the second ResolveImport caller (.sehgcc handler resolution) is covered by the same guard. The test's CallPersonality(0x1000, …) cases pin exactly the previously faulting invocation for both phases, and the if(WIN32) registration block mirrors the existing guest_bad_optional_access pattern.

One nit: Readable's loop advances by RegionSize and only terminates via bounds or a failed VirtualQuery; a committed region reporting RegionSize == 0 would in principle loop forever. Not reachable in practice, but a memory.RegionSize == 0 bail-out alongside the other checks would make the guard airtight.

@DavidSamir

Copy link
Copy Markdown
Contributor Author

Thanks @fogydes, good catch. 1fdd345499 makes Readable treat a region reported with RegionSize == 0 as unreadable, so the walk always ends. I also merged current main. The 31 exception, unwind, backtrace, overflow_error and bad_optional_access tests pass on Windows, and check_conventions.py passes.

AI-assisted: yes

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

The fix now composes correctly with merged #8087: the Readable checks protect the shared ResolveImport path used by both personality and .sehgcc handler lookup. The zero-size VirtualQuery region case raised by the independent reviewer is also handled. I found no remaining behavior defect in the Windows guard, and the Windows red/green results are clearly reported by the author. One repository gate remains: 1. Test registration overlap: #6044 remains open and reorganizes the same libc/CMakeLists.txt exception-test block. Its patch moves the existing exception_personality registration while this PR adds a Windows registration for that test. The overlap workflow could not compare #6044 because it conflicts with main; the description notes the hunk count but does not record an agreed merge order with that PR's author or a Depends on edge. Please agree which change lands first and record the order in the checklist, then rebase accordingly. #8087 is already merged, so it no longer needs to be listed as a dependency. This head is also four commits behind current upstream/main; please update it after the overlap order is agreed. Checks on exact head a3fa95b4d75b7c3ed5299216d98e102dd6d3ef91: - git diff --check upstream/main...HEAD: passed. - python tools/check_conventions.py --base upstream/main --head HEAD: passed. - The latest hosted relinker matrix, ASan, register-context and write-watch checks pass. Hosted Linux/Windows product jobs are skipped. - The Windows-only exception_personality target is not available to run in this Linux workspace. The Windows test results and negative control in the PR description are author-reported; the independent review confirms the guard behavior and regression case. I am requesting changes for required overlap coordination and base freshness only, not a queue-based intake hold or a defect in the guarded read itself. It is fine to keep submitting PRs; maintainers will handle them in due course.

I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.

@DavidSamir

Copy link
Copy Markdown
Contributor Author

Thanks @MarcZX10 for the careful passes, and for confirming no remaining behaviour defect.

On intake: we're keeping this open under the same reasoning as on #9379. It's a small fix/test with negative controls and no inferred title. If a maintainer asks on this PR to hold it, I'll convert it to a draft right away. I'll merge main again just before it's picked up, so the final matrix runs on the current base without a merge commit every few hours. For #6044 (the exception_personality registration it moves), I proposed the order on #6044: neither waits, and if #6044 lands first the Windows registration follows the moved file. I'll record the reply in the checklist. #8087 is merged and no longer listed.

AI-assisted: yes

@JJenkx

JJenkx commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Independent Windows run, as data only. Machine: Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Release. Head a3fa95b was merged onto main 8b3a6ba.

AI-assisted (Claude Code).

@MarcZX10

Copy link
Copy Markdown
Contributor

Correction to my earlier review: please disregard its intake objection and request to defer. Given your contribution-count eligibility, a queue-based hold does not apply, and I am not asking you to infer a title need.

The actionable review items are base freshness and the test-registration overlap. This head is 52 commits behind current upstream/main 8b3e39832af85f404c6195418d11b3e1ebd1924d; git diff --check and current-main conventions pass. The author and independent reviewer report the Windows regression and negative control passing; I cannot run that Windows-only test in this Linux workspace, and hosted product jobs are skipped. #6044 is still open and its author has not replied to the proposed ordering posted there, so please record the agreed handling of the exception_personality registration before updating/retesting against current main. I found no new defect in the guarded read.

@MarcZX10

Copy link
Copy Markdown
Contributor

I’ve noticed that my earlier request to hold this PR based on #5239 was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. Please disregard that request.

@MarcZX10

Copy link
Copy Markdown
Contributor

I noticed the CMake test-registration hunks in #9585 and #9775 share the insertion area with #10608, which adds the guest_underflow_error test. The overlap bot reported the shared core/libs/prx/libc/CMakeLists.txt anchor after #10608 was approved. Could we agree and record a landing order across these registrations? They appear independent, so whichever PR lands second can apply its registration against the updated CMake file; please confirm that with the relevant authors and reflect it in the checklists.

@adriadam10 adriadam10 self-assigned this Oct 25, 2026
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 26, 2026
…n Windows

CallPersonality followed a jmp [rip+disp] import thunk by reading the bytes
at the personality address before checking it against the known
personalities, so an unmapped personality faulted instead of returning
_URC_FATAL_PHASE1_ERROR / _URC_FATAL_PHASE2_ERROR as it does on Linux. The
thunk and its slot are now read only when VirtualQuery reports them
committed and readable. exception_personality, which covers this, now runs
on Windows too.
Readable advanced by each region's size, so a region reported with size 0
would never end the walk. It now counts as unreadable.
@adriadam10
adriadam10 force-pushed the fix/win-unknown-personality branch from a3fa95b to 1d396a2 Compare October 26, 2026 09:29
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 26, 2026
@adriadam10
adriadam10 dismissed MarcZX10’s stale review October 26, 2026 10:18

Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.

@adriadam10
adriadam10 merged commit 4e38e23 into boykopovar:main Oct 26, 2026
17 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the Readable walk (terminates on empty regions, covers the thunk bytes and the IAT slot, shared by CallPersonality and VirtualUnwind through ResolveImport) and the if(WIN32) registration of exception_personality; the head merges cleanly onto current main, and the Windows results and negative control are confirmed by @JJenkx; merged.

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.

5 participants