Skip to content

fix(libkernel): isolate Windows positioned file transfers - #2029

Closed
STUPIDCONTEM wants to merge 1 commit into
boykopovar:mainfrom
STUPIDCONTEM:fix/windows-positioned-io
Closed

STUPIDCONTEM wants to merge 1 commit into
boykopovar:mainfrom
STUPIDCONTEM:fix/windows-positioned-io

Conversation

@STUPIDCONTEM

@STUPIDCONTEM STUPIDCONTEM commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Evidence review (2026-10-09)

Draft pending the requested title-specific failure evidence and before/after title load-time measurements. The synthetic concurrency regression does not establish a game benefit or justify per-transfer reopen/query overhead. The #2423 probes supply no title ID, so their separate error-return changes are not being folded into this PR.

Game/title ID: none verified. No PS5 title execution is claimed.

What

Concurrent positional I/O on Windows saved and restored the shared file pointer. A simultaneous sequential read could observe the positional transfer's offset, and restoring it could discard another thread's progress.

NativePositioned now uses a separately reopened file object for each transfer. It checks the original handle's granted access before reopening, so a read-only descriptor cannot gain write access (or vice versa). The original file pointer is never moved. EOF behavior is retained; reopen failures return an I/O error without a seek-and-restore fallback.

The new guest_positioned_io regression exercises all four scalar/vector positional APIs concurrently with sequential reads, including a duplicated descriptor. It checks exact file contents through an independent native reader, permission failures, short reads, EOF, zero-length transfers, and exclusive-sharing failures. TechnicalDebt records the sharing/locking limitation.

Host behavior follows Microsoft's ReOpenFile and NtQueryObject documentation.

Scope: synchronous positional I/O only. #1135 concerns the separate AIO implementation. Inspected #1777 and #1518; their Stdio.cpp changes address random devices and timestamps rather than this helper.

Tested

Windows 10 Pro 19045, WinLibs MinGW-w64 GCC 15.2.0 UCRT/posix/SEH r7, Release, Ninja, BUILD_TESTING=ON, SPIRV-Tools disabled. Base: f145139676a48306681d3a4d5fabf83938759daf.

  • Before the fix: the new concurrency regression fails at concurrent transfers preserve sequential data and positioned results; existing guest_filesystem and guest_file_vector pass.
  • With the fix: expanded concurrency/permission/EOF regression passes five consecutive runs (~0.33 s each).
  • Final regression also includes native full-file verification and exclusive-sharing failures.
  • Full default build and explicit libs build succeed.
  • Full CTest suite: 489/489 passed, 55.75 s, including the final regression and GPU execution tests.
  • Committed-head conventions check, whitespace check, and test-registration check pass.

Linux and a PS5 title/homebrew were not run. No game compatibility or performance improvement is claimed. Reopening for each transfer has overhead; restrictive host sharing/locks can prevent a transfer.

Overlap review: #1807 adds an adjacent sandbox documentation entry; there is no functional dependency. #2001 also changes NativePositioned, but removes the shared-position save/restore while retaining transfers on the original handle; it does not implement the separate-handle isolation in this PR. This PR remains independently based on main; those overlapping hunks need reconciliation if either change lands first.

An external review approved this exact head and reports a Linux/GCC 12.2 build plus passing guest_positioned_io (1/1). This is reviewer-reported validation, separate from the local Windows results above.

Checklist

  • Based on current main; no other open PR implements the same change (historical base; current readiness not asserted)
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw; no silent stubs added
  • No notes, investigation files or images added to the repository
  • New third-party code is a submodule built from source (none added)
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes — OpenAI Codex assisted with implementation, testing, and this description.

@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 the complete diff at 458e7e4. The Windows path no longer seeks on the shared descriptor: it checks granted data access, reopens a separate handle, uses the requested offset, and closes it. The tests cover concurrency on a duplicated descriptor, sequential offset preservation, access modes, EOF, and Windows sharing failure. Exact-head Linux/GCC 12.2 build and guest_positioned_io passed (1/1, 0.03 s); conventions and diff checks pass. The author reports five Windows regression runs and full CTest 489/489 on Windows. GitHub reports no checks for this head, and I could not execute the Windows-specific path here.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Which title hits this race? Reopening the file adds a ReOpenFile and an NtQueryObject call to every pread/pwrite on Windows, so this needs a title that shows the problem and its load time before and after.

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

The new #2423 report overlaps the same Windows NativePositioned helper changed here: the contributor reports that sceKernelPread/sceKernelPwrite failures throw across the guest boundary and abort, and sceKernelFtruncate also throws instead of returning an SCE error. Their proposed patch notes #2029 requires reconciliation, so I have not started a parallel change. Could you coordinate with #2423’s reporter to fold compatible errno/error-return handling into this branch or a narrowly scoped follow-up after this PR lands? Please also resolve the new overlap warning against #2144, #2212 and #2323 if their TechnicalDebt hunks conflict.

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

I prepared a local integration candidate based on your current head 458e7e42, commit 1a91cad331b41349ca029f78cc16913b277887b4 (not pushed). It preserves the separate ReOpenFile handle and changes the Windows scalar APIs to return SCE errors; sceKernelFtruncate now has its own SCE mapping while ftruncate_nid_postfix keeps POSIX -1/errno semantics. Linux libkernel and guest_positioned_io pass (1/1), and Conventions/diff checks pass. Windows validation is still required on this integrated head. Because this is your fork branch, I have not pushed the commit. If you want it integrated, please cherry-pick it or confirm that you want me to push to your branch.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 9, 2026
@STUPIDCONTEM
STUPIDCONTEM marked this pull request as draft October 10, 2026 00:38
@STUPIDCONTEM

Copy link
Copy Markdown
Contributor Author

@oneandonlydean I cannot currently provide a title ID or before/after title load-time measurements. The regression demonstrates the shared-offset race, but does not establish a game benefit or justify the per-transfer ReOpenFile/NtQueryObject cost. I have returned this PR to draft pending that evidence.

@MarcZX100 Thanks for identifying the #2423 overlap and preserving the separate file handle in your candidate. I will keep this branch focused on its original scope; please do not push the combined error-return changes into it. #2041 was explicitly declined pending a title ID and a log at the resize throw, and #2423's probes do not supply that missing title evidence. No combined Windows validation is claimed.

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

Labels

build docs libkernel needs rebase Conflicts with main, rebase needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants