fix(EntityEnhancements): guard eye detours against re-entrancy (StackOverflow crash) - #115
Open
Prefix wants to merge 1 commit into
Open
fix(EntityEnhancements): guard eye detours against re-entrancy (StackOverflow crash)#115Prefix wants to merge 1 commit into
Prefix wants to merge 1 commit into
Conversation
…ainst re-entrancy The four PointViewControl eye detours resolve managed wrappers while running inside the hooked native function -- MakeEntityFromPointer<IPlayerPawn>() on entry, and ShouldOverrideEye()'s CameraService.ViewEntity, which calls BaseEntity.Create(). When any of that work reaches the hooked function again the detour re-enters itself and recurses until the stack is exhausted. The process then dies with System.StackOverflowException, which .NET does not allow to be caught -- the server goes down with no recoverable error. Observed in production on a server whose plugins call IBasePlayerPawn .GetEyePosition() frequently: two crash reports, ~17,200 identical frames of LinuxGetEyePosition each, innermost frames MakeEntityFromPointer -> BaseEntity.Create. Both on a build that already contains the vtable-resolved hook from Kxnrl#109, so that change does not address this. Servers that never call GetEyePosition never execute the detour body and so never see it, which is why this has gone unnoticed. Fix is a [ThreadStatic] re-entrancy flag: if the detour is already active on this thread, go straight to the trampoline. Thread-static because the hooked function is not main-thread-only. try/finally so the flag is cleared even if wrapper resolution throws -- MakeEntityFromPointer throws InvalidOperationException("nullptr") when the pointer does not resolve. Applied to all four detours (Linux + Windows, position + angles) since they share the same shape.
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
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.
The four
PointViewControleye detours resolve managed wrappers while running inside the hooked native function:_entityManager.MakeEntityFromPointer<IPlayerPawn>(pEntity)ShouldOverrideEye()→pawn.GetCameraService()?.ViewEntity→BaseEntity.Create()If any of that work reaches the hooked function again, the detour re-enters itself and recurses until the stack is exhausted. The process then dies with
System.StackOverflowException, which .NET does not permit catching — the server goes down with no recoverable error and no log line.Evidence
Two managed crash reports from a production server, six days apart, identical signature:
The second report shows 17,208 frames of the same function. Both are on a build that already contains the vtable-resolved hook from #109, so that change does not address this.
Note
ShouldOverrideEye's second operand is evaluated on every call whenever_viewControlEntitiesis empty — i.e. on every server that has nopoint_viewcontrolentity but does callGetEyePosition.Why it is rarely seen
The hook installs unconditionally at
Init(), but the detour body only runs when something actually callsGetEyePosition/GetEyeAngles. Servers whose plugins never call them never execute it. It reproduces reliably on a server whose plugins callIBasePlayerPawn.GetEyePosition()frequently.Fix
A
[ThreadStatic]re-entrancy flag — if the detour is already active on this thread, go straight to the trampoline.MakeEntityFromPointerthrowsInvalidOperationException("nullptr")when the pointer does not resolve, which would otherwise latch the flag permanentlyBehaviour is unchanged for every non-re-entrant call. Builds clean against
master(0 Warning(s), 0 Error(s)).Possible follow-up (not in this PR)
The hook is installed even when the map contains no
point_viewcontrolentity, so on most servers it is pure overhead on a hot path. Deferring installation until the first such entity spawns would avoid the cost entirely — happy to do that separately if you would like it.