Prefab staging: fix the map-heap lifetime bug, and enable Load/Place auto-grab - #53
Open
mefisme wants to merge 2 commits into
Open
Prefab staging: fix the map-heap lifetime bug, and enable Load/Place auto-grab#53mefisme wants to merge 2 commits into
mefisme wants to merge 2 commits into
Conversation
… map changes
A prefab staged by Load/Place did not survive a Play round-trip: coming back and
pressing Ctrl+V pasted nothing, and the next Ctrl+C faulted inside the engine
while CreatePrefab tore the slot down. It was mitigated by discarding our staged
prefab on the way into Play; this fixes the cause instead.
ROOT CAUSE. Not structural inequivalence -- the object we deserialize is
member-for-member identical to CreatePrefab's. Its entity-blob array was
allocated in the engine's MAP heap. Read live from each block's own allocator
header:
engine Ctrl+C clipboard, 1 entity tag 0x05 flags 0x00 432 B -> process heap
our staged prefab, 5 entities tag 0x05 flags 0x04 2160 B -> MAP heap
Same element type, same tag, different heap. The engine keeps three heaps
(global / persist / map) and destroys the map heap with HeapDestroy at map load,
so our array's pages were returned to the OS while the prefab's idList header
still pointed at them with num == capacity. Measured directly: after Play the
header was byte-identical but reading blob byte 0 FAULTED -- unmapped, not freed.
That distinction is what identified the mechanism, since a freed block's pages
stay committed and read as garbage.
It also explains why one bug produced two symptoms: touched while still unmapped
gives an access violation; touched after unrelated allocations re-commit that
range gives plausible garbage with the heap-owned bit set, which fails the
allocator's guard-cookie check and raises the fatal "Memory corruption before
block!".
WHY IT LANDS THERE. Every idlib container asks Mem_Alloc for heap id -1, meaning
"whatever is on top of idMemLocal's heap-scope stack". An allocation's lifetime
therefore depends on WHEN it runs, not what is allocated -- and while the SnapMap
editor is up the engine already keeps the map heap pushed (observed scope depth 1).
Our deserialize inherited it.
FIX. Wrap the stage in the engine's own scope API, idMemLocal::PushHeap(0) /
PopHeap(). Heap-table slot 0 is NULL and Mem_Alloc falls through to
GetProcessHeap() when the selected handle is null, so pushing 0 puts the blob
array exactly where the engine's own clipboard lives. The engine uses this same
idiom itself. No member copying changed.
sh_apply_prefab_poll_play then stops discarding our prefab -- but asks the block
rather than assuming: ae_block_survives_map validates the allocator's guard cookie
and reads the owning heap out of the block header, returning 0 for anything it
cannot verify. If the push ever stops working the protective re-ctor runs again,
so the failure mode is the old behaviour, not a crash.
ADDRESSING. MemLocalGet / MemLocalPushHeap / MemLocalPopHeap are resolved by
SIGNATURE with known_rva as a cross-check only, like every other engine leaf here.
There is deliberately no raw-address path to the instance: an earlier revision read
it from a hardcoded data RVA, which cannot be signature-verified and gets CALLED
through -- and a dropped digit in that address (0x155B7190 for 0x55B7190, ~256 MB
past the mapped module) made the scope push a silent no-op until it was caught.
Every call is additionally range-checked against the DOOM module, because reading a
bad pointer is recoverable via SEH and calling one is not.
Two properties of the mechanism, from the engine's code rather than assumed: it is
MAIN-THREAD-ONLY (PushHeap, PopHeap and Mem_Alloc's -1 lookup share a
GetCurrentThreadId gate, and are silently inert elsewhere), and the scope stack is
GLOBAL rather than per-thread -- which is a leak risk, never corruption, because
each block records its own heap and Mem_Free reads it back, so a block is always
freed into the heap it came from. PopHeap fatals on underflow, hence __try/__finally.
Verified live: the staged prefab now survives both a Play round-trip and a map
change, with Ctrl+V working afterwards, matching the engine's own clipboard.
Also included: the diagnostics that established all of the above (staging,
Play-round-trip and paste-outcome probes), all compiled out by default with their
findings and one known trigger defect documented at each flag. Both flag states are
build-tested, since a break behind a #if is invisible until someone flips it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Load/Place now stages AND picks the prefab up -- no Ctrl+V needed. The user
positions it and clicks to drop, exactly as after a vanilla Ctrl+V; that click
goes to the engine's own place-commit handler untouched.
The mechanism was already implemented and proven: injecting the editor's own
abstract paste action (0x5C) into the mode object's action slot, so the engine runs
its real paste branch rather than anything reimplemented. Enabling it is one
constant. It was left disabled because an unexplained "Memory corruption before
block!" had followed two auto-grab pastes while repeated manual pastes looked
clean, with two suspects on record: that we SET the paste-available bit rather than
letting the engine recompute it, and that we ClearSelection immediately before
arming.
Both are now eliminated, and the corruption was the map-heap bug fixed in the
previous commit -- which was live during every earlier auto-grab observation.
Evidence:
- eight auto-grab pastes across two sessions, three of them on a FRESH map
followed by several minutes of ordinary editing, produced no corruption;
- post-paste editor state is IDENTICAL to a manual Ctrl+V:
mode+0x1ac=4 arm(+0x420)=-1 action(+0x424)=0x0
flags1(+0x41)=0x64 pasteAvail=1 flags2=0x00 dirty=0
so neither suspect leaves a residue, and the arm word self-clears -- an
injected action cannot re-fire.
Note the "auto-grab specifically is at fault" reading always rested on a manual
control whose two recorded accounts contradicted each other, one saying manual
pastes of our prefab corrupted and the other saying they were clean.
WE DO NOT BYPASS A CAPACITY CHECK. Read from the engine's gate recompute, the
paste-available bit is exactly two conditions -- the copy/paste cvar AND staged
entity count >= 1 -- inside the nothing-hovered branch. No budget term. And because
this route injects an action rather than calling PasteInstantiate directly, the
engine still runs its own paste branch and every check inside it. Separately
confirmed: the in-editor budget rows are SOFT (the editor deliberately allows
exceeding them), so they are not something to gate on either.
The one honest residue: we set the bit without reading snapEdit_enableCopyPaste, so
a paste is possible while the user has copy/paste disabled. Bounded -- the engine's
next recompute clears the bit.
Degradation is unchanged and still load-bearing. Any of "not in EntityMode",
"already holding" (mode+0x1ac == 4), "hovering an entity", or "selection could not
be cleared" aborts to stage-only with a toast explaining which. All were observed
firing during the test, including five consecutive mode-busy refusals while the
editor was holding.
Docs cover both this and the heap fix: capabilities.md describes Load/Place as
picking the prefab up (with its degrade conditions) and as surviving Play and map
changes, instead of telling the user to press Ctrl+V; backend-changes.md gains both
entries, including the unattributed BU_STATIC renderer event seen once under
deliberately heavy load and why it is not attributed to either change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Two commits: the heap fix, then the one-constant change that it unblocked.
The fix
A staged prefab used to be discarded on the way into Play, because coming back and pressing Ctrl+V pasted nothing and the next Ctrl+C faulted in
CreatePrefab's teardown. The recorded reason was that our deserializedidSnapEntityPrefabwasn't structurally equivalent to oneCreatePrefabbuilds.It's equivalent. The problem was which heap its entity-blob array came from. Every idlib container asks
Mem_Allocfor heap id-1— "top ofidMemLocal's heap-scope stack" — and while the editor is up that's the map heap, which the engineHeapDestroys at map load. So the array's pages were freed while the prefab still pointed at them. The engine's own clipboard lands in the process heap, which is why it survives.Fix: wrap the stage in the engine's own
idMemLocal::PushHeap(0)/PopHeap(), which is the idiom the engine uses itself. No member copying changed.sh_apply_prefab_poll_playthen stops discarding the prefab, but verifies rather than assumes —ae_block_survives_map()reads the owning heap out of the block's allocator header and validates its guard cookie, returning false for anything it can't confirm. If the push ever stops working, the old protective re-ctor runs, so the failure mode is the previous behaviour rather than a crash.Auto-grab
The mechanism was already implemented and proven (inject the editor's own paste action
0x5C, so the engine runs its real paste branch). It was disabled for aMemory corruption before block!that had followed two auto-grab pastes.That corruption was the heap bug above, which was live at the time. Re-tested after the fix: 8 auto-grab pastes across two sessions, three on a fresh map, no corruption — and post-paste editor state is identical to a manual Ctrl+V, which rules out the two things we do differently (we set the paste-available bit rather than waiting for the engine's recompute, and we clear the selection before arming).
Worth stating since it's the obvious question: this doesn't bypass a capacity check. The paste-available bit is exactly two conditions — the copy/paste cvar and staged entity count ≥ 1 — with no budget term. And because we inject an action rather than calling
PasteInstantiatedirectly, the engine still runs its own paste branch and everything in it.Degradation is unchanged: not in EntityMode, already holding, hovering an entity, or a selection that wouldn't clear all abort to stage-only with a toast saying which.
Addressing
Given #51 was about not trusting stale pinned RVAs:
known_rvaas a cross-check only, like every other leaf.Testing
tests\run-tests.ps1 -Doom <unpacked exe>— 16/16 native, signature resolver 62/62 unique, 0 RVA mismatches, hook-tolerant 3/3.go test ./...andgo vet ./...ininstaller/— clean.build.ps1+package.ps1— clean.Diagnostics used to find this are included but compiled out by default; both flag states are build-tested.
Two things to know
snapEdit_enableCopyPaste, so a paste is possible with copy/paste disabled. Bounded — the engine's next recompute clears it.Cannot map buffer with usage BU_STATICand returned to the map-select menu — its documented recoverable path, not corruption. Didn't reproduce on a fresh map, isn't linked to this change, andsnapEdit_skipLimitswas off. Mentioning it only so it isn't a surprise.