Skip to content

Fix BaseExtraList::RemoveExtra leaving _tail inside the removed node - #70

Closed
ejams1 wants to merge 2 commits into
libxse:mainfrom
ejams1:fix/baseextralist-removeextra-tail
Closed

ejams1 wants to merge 2 commits into
libxse:mainfrom
ejams1:fix/baseextralist-removeextra-tail

Conversation

@ejams1

@ejams1 ejams1 commented Oct 3, 2026

Copy link
Copy Markdown

BaseExtraList::_tail points at the last node's next field (AddExtra does *_tail = a_extra; _tail = &a_extra->next). In a consistent list *_tail is therefore always nullptr, so the check in RemoveExtra never holds:

if (!_tail || *_tail == iter) {

When the removed node is the last one, which is the usual position for a non-high-use type such as ExtraInstanceData, _tail is left at &removed->next. The next AddExtra of a non-high-use type writes through it, into a node that is no longer in the list. If that node is the one just removed (remove, try to rebuild, put the old one back), it ends up pointing at itself. BaseExtraList::RemoveAllDefault then walks a cycle on the next revert. With one allocator it frees the same block forever; with the stock heap it calls through the freed object's cleared vtable (Fallout4.exe+0272571 call [rax]).

The change:

  • The test is _tail == std::addressof(iter->next), so _tail moves back to prev->next (or _head) exactly when the last node is removed.
  • The removed node's next is cleared. A node removed from the middle still pointed into the list, so handing it back to AddExtra (which asserts next == nullptr) would link the rest of the list behind it.

We found this through a plugin that refreshes reference instance data with RemoveExtra(kInstanceData) and re-adds the old node when the rebuild fails. Every save load then hung or crashed in RemoveAllDefault.

_tail points at the last node's next field, so *_tail is always null in a consistent list and the old test (*_tail == iter) never held. Removing the last node left _tail at &removed->next, and the next AddExtra of a non-high-use type wrote through it; re-adding the removed node made it point at itself, and BaseExtraList::RemoveAllDefault then looped or called through a freed object. The test is now _tail == &iter->next, and the removed node's next is cleared so it can be handed back to AddExtra.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@ejams1
ejams1 marked this pull request as ready for review October 3, 2026 00:20
@ejams1 ejams1 closed this by deleting the head repository Oct 3, 2026
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