ansi: fix margin writer closures capturing stale block-stack buffer - #594
Open
lfaoro wants to merge 1 commit into
Open
ansi: fix margin writer closures capturing stale block-stack buffer#594lfaoro wants to merge 1 commit into
lfaoro wants to merge 1 commit into
Conversation
The PadFunc and IndentFunc closures in NewMarginWriter captured the constructor's w argument (a *bytes.Buffer from the renderer's block stack) and ignored their io.Writer parameter. This caused padding and indent writes to bypass the WrapWriter chain and land directly on the block-stack buffer, which is reused across renders. Under concurrent GC pressure this produced heap corruption (pointer to unallocated span) and/or goroutine hangs. Fix: route padding/indent writes through the io.Writer parameter so they pass through the proper WrapWriter chain. Add regression test TestMarginWriterHeapCorruption that exercises the WithPreservedNewLines + Document.Margin trigger with 5000 iterations (reproduces the crash in ~50% of runs without the fix). Update golden files to reflect corrected output (2-space margin now properly applied through the writer chain).
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.
Fix: margin writer closures capture stale block-stack buffer
Problem
NewMarginWriterinansi/margin.gocreatesPadFuncandIndentFuncclosures that capture the constructor'swargument (a*bytes.Bufferfrom the renderer's block stack) and ignore theirio.Writerparameter. WhenPaddingWriter.WriteandIndentWriter.Writecall these functions, padding and indent characters are written directly to the block-stack buffer, bypassing thelipgloss.WrapWriterchain.The block-stack buffer is reused across renders. Under GC pressure, stale pointers from closed-over captures collide with buffer reuse, producing:
runtime: pointer ... to unallocated spanheap corruptionTrigger
Both conditions must be true:
WithPreservedNewLines()β newlines reach the margin writers instead of being collapsed to spacesStyleBlockwithMargin != nilβ engages the margin/indent/padding writer chainWhen both are active, every
\nin paragraph text triggersPaddingWriter.Write's padding branch, which callsPadFuncon the bypass path.The default
DarkStyleConfigand all built-in styles setDocument.Margin = uintPtr(2), so the margin writers are always engaged.WithPreservedNewLinesis the additional trigger.Reproduction
Happens probabilistically (~50% of runs without
-race; never with-raceor single-package execution). Stack always traces through:A minimal repro run 50 times:
Fix
Change both closures in
NewMarginWriterto use theirio.Writerparameter:Now padding and indent writes pass through the proper
WrapWriterchain, which correctly manages ANSI state and cursor position.Changes
ansi/margin.go: 4-line closure fixglamour_test.go: regression testTestMarginWriterHeapCorruptionTesting
go test -count=20 -run=TestMarginWriterHeapCorruptionβ passesgo test ./...β passesgo test -race ./...β passes