Fix dangling pointer crash in PacketReader(byte[], bool)#249
Open
kamronbatman wants to merge 1 commit intomarkdwags:masterfrom
Open
Fix dangling pointer crash in PacketReader(byte[], bool)#249kamronbatman wants to merge 1 commit intomarkdwags:masterfrom
kamronbatman wants to merge 1 commit intomarkdwags:masterfrom
Conversation
The byte[] constructor pinned the array only for the duration of the fixed block and stored the pointer past the unpin, leaving m_Data referring to a managed array the GC was free to relocate. Reads after a Gen0 compaction would dereference invalid memory and raise AccessViolationException. The most reliable trigger is GetCompressedReader -> CompressedGump, which allocates a string[] between constructing the inner reader and the first ReadInt16 -- opening a Gen0 collection window before any read. Pin the array for the lifetime of the reader via GCHandle and release it from the finalizer; suppress finalize on the byte* overload since the caller owns the pin there.
f5cca2a to
75e62cf
Compare
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 bug
PacketReader(byte[] buff, bool dyn)inRazor/Network/Packet.csstored abyte*past the lifetime of itsfixedblock:The instant the
fixedblock exits,buffis unpinned and may be relocated by the GC.m_Databecomes a dangling pointer; every subsequentReadBytedereferences whatever now lives at the old address — usually still-valid heap memory (silent), sometimes garbage from another allocation, sometimes an unmapped page.The reliable production trigger is
GetCompressedReader→CompressedGump. The handler allocatesnew string[numStrings]between constructing the inner reader and the firstpComp.ReadInt16(), opening a Gen0 collection window before any read happens.In .NET Framework 4+,
AccessViolationExceptionis a Corrupted State Exception. Razor's app.config doesn't enable<legacyCorruptedStateExceptionsPolicy>and no method carries[HandleProcessCorruptedStateExceptions], so the AVE escapes bothcatchblocks (CompressedGump's andProcessViewers's) and the process is terminated. This matches the reported production stack trace verbatim:Empirical proof
A standalone repro built two readers — the current buggy ctor and a
GCHandle.Pinnedvariant — fragmented the LOH with bracketing 100KB allocations, dropped the holes, forced a compacting Gen2 GC, then read every byte. 30 runs each on x64 Server GC, .NET 4.7.2:The buggy run produced the exact
AccessViolationExceptionatReadByteshown in the production crash. The fixed reader couldn't be relocated at all (GCHandle.Pinnedis what it says on the tin) and every byte read came back correct. Note the bug is observable in 2/30 runs as garbage or AVE; the other 28 are "lucky" — the freed memory at the stale address still happened to contain the original bytes. This explains why crashes feel rare in production despite the bug firing on every CompressedGump.The fix
Replace the leaked
fixedpointer with aGCHandle.Alloc(buff, GCHandleType.Pinned)taken in the ctor and freed in a finalizer. Thebyte*overload callsGC.SuppressFinalize(this)since the caller owns the pin in that path.The pin lasts at most one GC cycle past the reader's last reachable use (until the finalizer runs), which is well under a millisecond in practice. No callers need to change.