From 2c92d92959c464d1194c60e00bbce10c0e0e129d Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 10 Jul 2026 21:24:35 +0500 Subject: [PATCH] fix(summary): bound SummaryParser.buf on newline-less log floods (D3, #226) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SummaryParser.Feed accumulated bytes into p.buf until a '\n'. The display path caps an oversized tqdm line via displayLineMax (16 MB) in streamDisplayAndParse and drains the rest, but the drained bytes still flow through the TeeReader into Feed — so an ingestor emitting many MB of '\r'-redraws with no '\n' for the life of a (up to 1h) run grew p.buf without the display-side cap ever applying to the parser. Bound buf at parserLineMax (= displayLineMax; same package, referenced directly so the two paths can't drift). When the partial newline-less line passes the ceiling, drop it and enter drop-until-newline mode so the oversized line's tail is discarded too rather than parsed as a spurious fresh line; FlushLine honors the same state at EOF. A real banner line is tens of bytes, so newline-less content past 16 MB can never be one — dropping is safe and the parser still recovers to parse the closing banner once a '\n' finally lands. Adds a white-box test feeding a >2x-parserLineMax newline-less flood, asserting buf stays bounded and a real banner after the flood parses. Deferred finding D3 from the v0.8.0 review (#220). Low severity. Co-Authored-By: Claude Opus 4.8 --- internal/submit/summary.go | 52 +++++++++++++++++++++++++++++++-- internal/submit/summary_test.go | 45 ++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/internal/submit/summary.go b/internal/submit/summary.go index 0f19042c..6168e936 100644 --- a/internal/submit/summary.go +++ b/internal/submit/summary.go @@ -210,8 +210,28 @@ type SummaryParser struct { // genuinely 0 (early failure case) — Bugbot caught this on // PR #10 round 2. sawAnyField bool + + // droppingLine is set when buf outgrew parserLineMax before a + // '\n' arrived: the partial (newline-less) line was discarded + // (see Feed). It stays set until the terminating '\n' is seen, + // so the oversized line's tail is dropped too rather than + // parsed as a spurious fresh line. + droppingLine bool } +// parserLineMax bounds the partial (newline-less) content the +// parser buffers in buf. It mirrors displayLineMax (watch.go) +// exactly — same package, so we reference it directly and the two +// paths can't drift. Rationale: the display side drains a tqdm +// '\r'-redraw burst that outgrows displayLineMax rather than +// failing, but those drained bytes still flow through the +// TeeReader into Feed. Without a matching bound here, a +// pathological ingestor emitting many MB of '\r' redraws with no +// '\n' for the life of a (up to 1h) run would grow buf without +// limit. A real banner line is tens of bytes; newline-less content +// past this ceiling cannot be one, so Feed drops it. +const parserLineMax = displayLineMax + // NewSummaryParser returns an initialized parser. Caller's // goroutine owns it for the duration of the watch loop. func NewSummaryParser() *SummaryParser { @@ -229,10 +249,27 @@ func (p *SummaryParser) Feed(b []byte) { for { idx := bytes.IndexByte(p.buf.Bytes(), '\n') if idx < 0 { - // No complete line yet — wait for more input. + // No complete line yet. If the partial (newline-less) + // content has outgrown parserLineMax it cannot be a + // banner line — drop it and enter drop-until-newline + // mode so the oversized line's tail is discarded too, + // not mistaken for a fresh line. Bounds buf at + // parserLineMax + one Feed chunk regardless of how long + // the ingestor withholds a '\n'. + if p.buf.Len() > parserLineMax { + p.buf.Reset() + p.droppingLine = true + } return } line := p.buf.Next(idx + 1) // consume up to and including '\n' + if p.droppingLine { + // This '\n' terminates an oversized line whose head we + // already dropped; discard the buffered tail and resume + // normal parsing on the lines that follow. + p.droppingLine = false + continue + } p.feedLine(string(bytes.TrimRight(line, "\n"))) } } @@ -242,7 +279,18 @@ func (p *SummaryParser) Feed(b []byte) { // terminated without a final '\n' (rare but possible if the // container's stdout was killed mid-write). func (p *SummaryParser) FlushLine() { - if p.buf.Len() > 0 && !p.finalized { + if p.finalized { + return + } + if p.droppingLine { + // EOF landed mid-drop: buf holds the tail of an oversized + // line whose head was already discarded. Drop the tail too + // rather than parse it as a line. + p.buf.Reset() + p.droppingLine = false + return + } + if p.buf.Len() > 0 { p.feedLine(p.buf.String()) p.buf.Reset() } diff --git a/internal/submit/summary_test.go b/internal/submit/summary_test.go index 58ab6b73..9a34ce07 100644 --- a/internal/submit/summary_test.go +++ b/internal/submit/summary_test.go @@ -174,6 +174,51 @@ func TestSummaryParser_PostBannerLogsIgnored(t *testing.T) { } } +// TestSummaryParser_BufferBoundedOnNewlinelessFlood pins finding D3 +// (deferred from the v0.8.0 review): a pathological ingestor can emit +// many MB of tqdm '\r'-redraws with no '\n' for the life of a run. The +// display path drains those past displayLineMax, but the drained bytes +// still flow through the TeeReader into Feed — so without a matching +// bound the parser's buf would grow unbounded. Assert buf stays within +// parserLineMax under the flood, and that a real banner arriving after +// the flood terminates (a '\n' finally lands) still parses. +func TestSummaryParser_BufferBoundedOnNewlinelessFlood(t *testing.T) { + p := NewSummaryParser() + + // tqdm-style redraw: '\r' + progress text, never a '\n'. Feed well + // past parserLineMax in bounded chunks so we also exercise the + // across-Feed-calls accumulation, not just one giant Write. + chunk := []byte("\r" + strings.Repeat("#", 512*1024-1)) // 512 KiB, no '\n' + for total := 0; total <= parserLineMax*2; total += len(chunk) { + p.Feed(chunk) + if p.buf.Len() > parserLineMax { + t.Fatalf("buf grew to %d bytes after a newline-less flood, exceeds parserLineMax=%d", + p.buf.Len(), parserLineMax) + } + } + + // The flood is one newline-less line; none of it should have been + // mistaken for a banner. + if got := p.Result(); got != nil { + t.Fatalf("newline-less flood produced a non-nil Summary: %+v", got) + } + + // The pathological line finally terminates and a real banner + // follows. The parser must recover: drop the oversized line's tail, + // then parse the banner that comes after. + p.Feed([]byte("\n")) + p.Feed([]byte(realIngestorBanner)) + + got := p.Result() + if got == nil { + t.Fatal("banner after a newline-less flood did not parse; Result is nil") + } + if got.TotalRecords != 1234 || got.FailedRecords != 30 { + t.Errorf("post-flood banner parsed wrong: TotalRecords=%d FailedRecords=%d, want 1234/30", + got.TotalRecords, got.FailedRecords) + } +} + // TestStripANSI: the parser strips ANSI SGR codes from each line // before matching. Validate the regex handles common shapes. func TestStripANSI(t *testing.T) {