Skip to content

Savestate: don't let stale HLE helper threads mutate restored kernel state - #21988

Open
Arkadyzja wants to merge 1 commit into
hrydgard:masterfrom
Arkadyzja:savestate-forget-stale-helper-threads
Open

Savestate: don't let stale HLE helper threads mutate restored kernel state#21988
Arkadyzja wants to merge 1 commit into
hrydgard:masterfrom
Arkadyzja:savestate-forget-stale-helper-threads

Conversation

@Arkadyzja

Copy link
Copy Markdown

The bug

On savestate load, two sites destroy a pre-load host object that owns an HLEHelperThread without calling Forget() first. ~HLEHelperThread then runs __KernelDeleteThread() and kernelMemory.Free() using a thread id and entry-block address from before the load, against the freshly restored kernel state:

  • sceUtility: Do(p, accessThread)DoClass deletes the stale host object before recreating it from the stream.
  • scePsmf: Do(p, psmfPlayerMap) — the map serializer deletes every existing PsmfPlayer, and ~PsmfPlayer -> AbortFinish() does a raw delete finishThread.

When the stale id/block happens to be absent from the restored state, this only produces error spam (__KernelDeleteThread: thread ... does not exist, BlockAllocator: invalid free) — annoying but harmless. When the id or block has been recycled by the restored state, a live thread is terminated or a live allocation is freed, silently corrupting the state that was just loaded. Easiest to hit by loading a state while a savedata operation or PSMF player is active in the current session.

The fix

__IoDoState and __PsmfShutdown already handle this correctly by calling Forget() before deletion (which detaches the host wrapper from the kernel-side objects — those belong to the serialized state, not to the wrapper). This PR applies the same pattern to the two remaining sites:

  • sceUtility.cpp: Forget() + delete the stale accessThread before Do(p, accessThread) deserializes the replacement.
  • scePsmf.cpp: before Do(p, psmfPlayerMap) on MODE_READ, walk the existing map and Forget() each player's finishThread.

Risk

Load-path only; strictly removes writes to restored kernel state. Worst-case behavior change: a kernel-side thread record that was previously (incorrectly) freed is now left to the restored state to manage — i.e., exactly what the serializer intends.

…state

On savestate load, two sites delete a pre-load host object that owns an
HLEHelperThread without calling Forget() first, so ~HLEHelperThread runs
__KernelDeleteThread and kernelMemory.Free() with thread ids and block
addresses from before the load, against the freshly restored kernel
state:

- sceUtility: Do(p, accessThread) deletes the stale accessThread inside
  DoClass before recreating it from the stream.
- scePsmf: Do(p, psmfPlayerMap) deletes every existing PsmfPlayer, and
  ~PsmfPlayer -> AbortFinish() deletes its finishThread raw.

When the stale id or block happens to be absent from the restored state
this only logs errors ("... does not exist" / "BlockAllocator: invalid
free"). When it has been recycled, a live thread is terminated or a
live allocation is freed, silently corrupting the loaded state. Easiest
to hit by loading a state while a savedata operation or PSMF player is
active, into a session where those ids were reused.

__IoDoState and __PsmfShutdown already Forget() before deleting; do the
same at these two sites. Worst case behavior change is a leaked
kernel-side thread record where one was previously (incorrectly)
freed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant