From de1855f695b62db5b314ac3a04d1ed904937661e Mon Sep 17 00:00:00 2001 From: John Campion Jr Date: Mon, 7 Sep 2026 16:07:22 -0400 Subject: [PATCH] A mark list handed to a reader must not change under them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BufferLine.Marks returned the live List, and AddMark appended to it. A host doing shell integration reads that list from its own thread — asking where the prompt is, what the last mark was — while the parser is still emitting marks on whatever thread drains the pty. Enumerating a List across an append throws InvalidOperationException, and the reader crashes for having read a documented public property. Seen for real: an Avalonia host asking for the last mark on the line while a shell was starting up, on Windows CI, roughly one run in three. AddMark now publishes a new list instead of appending to the one already handed out, so a reader keeps a complete, consistent snapshot. The field is volatile: the reference swap is atomic either way, but without the release/acquire pair a reader on a weak memory model could see the new reference before the list it points at is fully built. Costs one small allocation per mark. Marks arrive a handful of times per command rather than per cell, so this is nowhere near the print path — and the copy is sized for its result, so the append cannot reallocate on top of it. The tests are deliberately thread-free. The property that fixes the race is "an append does not touch a list already handed out", which is exactly testable; both new tests fail on the old code, one with the production InvalidOperationException. Links, sized runs, placements and images have the same shape but are not the same change: those lists are edited in place by span splitting, not only appended to, and are read inside the render pass. Left alone deliberately. --- .../ShellIntegrationMarkAnchorTests.cs | 37 ++++++++++++++++++ src/XTerm.NET/Buffer/BufferLine.cs | 38 +++++++++++++++++-- 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/src/XTerm.NET.Tests/ShellIntegrationMarkAnchorTests.cs b/src/XTerm.NET.Tests/ShellIntegrationMarkAnchorTests.cs index 473b0d2..b93e5f4 100644 --- a/src/XTerm.NET.Tests/ShellIntegrationMarkAnchorTests.cs +++ b/src/XTerm.NET.Tests/ShellIntegrationMarkAnchorTests.cs @@ -225,4 +225,41 @@ public void With_no_prompts_there_is_nothing_to_jump_to() Assert.False(t.TryFindPreviousPrompt(t.Buffer.Lines.Length, out _)); Assert.False(t.TryFindNextPrompt(-1, out _)); } + + /// + /// The list hands back is a snapshot, so a host reading it can + /// finish reading it. A host does this from its UI thread while the parser is still writing on + /// whatever thread drains the pty; against a live list that append throws + /// mid-enumeration. + /// + /// Written without threads on purpose: the property that fixes the race is + /// "an append does not touch a list already handed out", and that is exactly testable. + /// A thread test would only reproduce it sometimes and would pass on the broken code. + /// + [Fact] + public void A_mark_list_stays_enumerable_while_more_marks_arrive() + { + var t = Fresh(); + t.Write(Mark("A")); + + using var reading = MarksOn(t, 0).GetEnumerator(); + Assert.True(reading.MoveNext()); + + t.Write(Mark("B")); + + Assert.False(reading.MoveNext()); + } + + [Fact] + public void A_mark_list_already_handed_out_does_not_grow() + { + var t = Fresh(); + t.Write(Mark("A")); + var handedOut = MarksOn(t, 0); + + t.Write(Mark("B")); + + Assert.Single(handedOut); + Assert.Equal(2, MarksOn(t, 0).Count); + } } diff --git a/src/XTerm.NET/Buffer/BufferLine.cs b/src/XTerm.NET/Buffer/BufferLine.cs index e5b08f0..a4d8345 100644 --- a/src/XTerm.NET/Buffer/BufferLine.cs +++ b/src/XTerm.NET/Buffer/BufferLine.cs @@ -24,7 +24,12 @@ public class BufferLine : IEnumerable /// /// Shell-integration marks on this line, or null — which is every line that is not a prompt. /// - private List? _marks; + /// + /// Volatile because publishes a new list rather than appending to this + /// one, and a reader on another thread must see the list fully built before it sees the + /// reference. Without the release/acquire pair that is not guaranteed on a weak memory model. + /// + private volatile List? _marks; /// /// OSC 8 link spans on this line, or null — which is nearly every line. @@ -471,6 +476,13 @@ public bool ClearImages() /// /// The shell-integration marks on this line, in the order they were emitted. /// + /// + /// A snapshot: the list handed back is never mutated afterwards, because + /// publishes a new one instead of appending to it. That is what makes this readable from a + /// thread other than the one writing to the terminal, which is what shell integration actually + /// does — a host asks where the prompt is while the parser is still emitting marks, and + /// enumerating a live across an append throws. + /// public IReadOnlyList Marks => (IReadOnlyList?)_marks ?? Array.Empty(); @@ -480,11 +492,31 @@ public IReadOnlyList Marks /// /// A line collects several: a prompt emits A and then B, and a command that produces no output /// finishes on the same line it started on. + /// + /// Costs one small allocation per mark, which the snapshot in is + /// worth: marks arrive a handful of times per command, not per cell, so this is nowhere near + /// the print path. /// internal void AddMark(LineMark mark) { - _marks ??= new List(1); - _marks.Add(mark); + // Copy on write, not Add — see Marks. Whoever already holds the published list keeps + // enumerating it unharmed; the reference swap is atomic, so a reader sees the old list or + // the new one and never a half-built one. Sized for the result so the Add below cannot + // reallocate on top of the copy. + var current = _marks; + List next; + if (current is null) + { + next = new List(1); + } + else + { + next = new List(current.Count + 1); + next.AddRange(current); + } + + next.Add(mark); + _marks = next; } ///