From 9b33d98a3607ab3e1cb056443d78c88d52b40d06 Mon Sep 17 00:00:00 2001 From: John Campion Jr Date: Mon, 24 Aug 2026 10:43:18 -0400 Subject: [PATCH] Suppress no-op colour events, reject bad indexes, and make reads coherent Three problems, one found in review and two found probing around it. NO-OP EVENTS. ResetAllColors raised ColorChanged whether or not anything had changed, so a bare OSC 104 on an untouched palette told a renderer to repaint for nothing -- and unlike every other setter here, which all suppress. ApplyTheme had the same flaw. Both are silent now when the values already match. CLAMPED INDEXES. The bigger one. Clamping mapped any out-of-range index onto 0 or 255, so SetColor(999, ...) quietly rewrote entry 255 and the indexer answered for entry 0 when asked about -1. A caller with an off-by-one got no error and a corrupted palette. Clamping is the one response that produces a plausible WRONG answer where there should have been none; it throws now. The OSC path is unaffected, because InputHandler range-checks first -- which is exactly why this went unnoticed, and why the existing test did not catch it. COHERENT READS. ApplyTheme copied into the live array, and Array.Copy is not atomic, so a renderer scanning the palette could paint a frame half in the old theme and half in the new. Bulk changes now build a snapshot and swap one reference, which has no middle. That alone was not enough, and measuring said so: a reader calling the indexer eight times takes eight separate snapshots and can still straddle a swap. Under a tight toggle that produced three and a half MILLION mixed reads in three seconds -- routine, not a rare race. So Take() hands out one immutable view, which is what to use when reading more than one colour, and for a renderer painting a frame that is always. Writes copy on write to keep it immutable: an int store is atomic and would have been safe for the value, but it would move a snapshot somebody is holding, and not moving is the entire point. A palette is 1KB and colour changes are rare. Reads stay lock free. The lock only serialises writers, because the indexer is on a per-cell path where locking would be a worse cure than the problem. 656 tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- src/XTerm.NET.Tests/ColorPaletteTests.cs | 178 +++++++++++++++++ src/XTerm.NET/Common/ColorPalette.cs | 232 ++++++++++++++++++++--- 2 files changed, 380 insertions(+), 30 deletions(-) diff --git a/src/XTerm.NET.Tests/ColorPaletteTests.cs b/src/XTerm.NET.Tests/ColorPaletteTests.cs index 357ce17..1d36b93 100644 --- a/src/XTerm.NET.Tests/ColorPaletteTests.cs +++ b/src/XTerm.NET.Tests/ColorPaletteTests.cs @@ -1,3 +1,5 @@ +using System.Threading; +using System.Threading.Tasks; using XTerm; using XTerm.Common; using XTerm.Options; @@ -281,6 +283,182 @@ public void ColorChanged_FiresForSetsAndNotForNoOps() Assert.Equal(0xFF0000, change.Rgb); } + // ---- no-op suppression and index handling ------------------------------------------------ + + [Fact] + public void ResetAllColors_IsSilent_WhenNothingHasChanged() + { + // Every other setter here suppresses a no-op. A bare OSC 104 on an untouched palette used to + // fire anyway, telling a renderer to repaint for a change that did not happen. + var terminal = CreateTerminal(); + var changes = new List(); + terminal.Colors.ColorChanged += (_, e) => changes.Add(e); + + terminal.Write("\u001b]104\u0007"); + + Assert.Empty(changes); + } + + [Fact] + public void ResetAllColors_StillFires_WhenSomethingHadChanged() + { + var terminal = CreateTerminal(); + terminal.Write("\u001b]4;1;#ff0000\u0007"); + + var changes = new List(); + terminal.Colors.ColorChanged += (_, e) => changes.Add(e); + + terminal.Write("\u001b]104\u0007"); + + var change = Assert.Single(changes); + Assert.Equal(ColorTarget.Indexed, change.Target); + Assert.Equal(0xCD0000, terminal.Colors[1]); + } + + [Fact] + public void ApplyTheme_IsSilent_WhenTheThemeIsUnchanged() + { + var terminal = CreateTerminal(new ThemeOptions { Background = "#ffffff" }); + var changes = new List(); + terminal.Colors.ColorChanged += (_, e) => changes.Add(e); + + terminal.Colors.ApplyTheme(new ThemeOptions { Background = "#ffffff" }); + + Assert.Empty(changes); + } + + [Fact] + public void ApplyTheme_FiresOnceForARealChange() + { + var terminal = CreateTerminal(new ThemeOptions { Background = "#000000" }); + var changes = new List(); + terminal.Colors.ColorChanged += (_, e) => changes.Add(e); + + terminal.Colors.ApplyTheme(new ThemeOptions { Background = "#ffffff" }); + + Assert.Equal(ColorTarget.All, Assert.Single(changes).Target); + } + + [Theory] + [InlineData(-1)] + [InlineData(256)] + [InlineData(999)] + public void PaletteIndex_OutOfRange_Throws(int index) + { + // Not clamped. Clamping made SetColor(999, ...) quietly rewrite entry 255 and the indexer + // answer for entry 0 when asked about -1: a plausible wrong answer where there should have + // been none. + var terminal = CreateTerminal(); + + Assert.Throws(() => terminal.Colors[index]); + Assert.Throws(() => terminal.Colors.SetColor(index, 0x123456)); + Assert.Throws(() => terminal.Colors.ResetColor(index)); + } + + [Fact] + public void PaletteIndex_OutOfRangeOverOsc_IsStillIgnoredRatherThanThrown() + { + // The parser must not surface a malformed sequence as an exception; InputHandler range-checks + // before it reaches the palette, and that has to keep working now the palette throws. + var terminal = CreateTerminal(); + + var ex = Record.Exception(() => terminal.Write("\u001b]4;999;#ffffff\u0007")); + + Assert.Null(ex); + Assert.Equal(0xEEEEEE, terminal.Colors[255]); + } + + // ---- concurrency ------------------------------------------------------------------------- + + [Fact] + public async Task ApplyTheme_IsNeverObservedHalfApplied() + { + // The bug this pins: ApplyTheme used to Array.Copy into the live array, and Array.Copy is not + // atomic, so a renderer scanning the palette could paint a frame that was half one theme and + // half the other. A reference swap has no middle. + var dark = new ThemeOptions + { + Black = "#000000", Red = "#110000", Green = "#001100", Yellow = "#111100", + Blue = "#000011", Magenta = "#110011", Cyan = "#001111", White = "#111111", + }; + var light = new ThemeOptions + { + Black = "#ffffff", Red = "#ff0000", Green = "#00ff00", Yellow = "#ffff00", + Blue = "#0000ff", Magenta = "#ff00ff", Cyan = "#00ffff", White = "#ffffff", + }; + + var terminal = CreateTerminal(dark); + int[] darkAnsi = ReadAnsi(terminal); + terminal.Colors.ApplyTheme(light); + int[] lightAnsi = ReadAnsi(terminal); + Assert.NotEqual(darkAnsi, lightAnsi); + + using var stop = new CancellationTokenSource(TimeSpan.FromSeconds(3)); + var mixed = 0; + + var reader = Task.Run(() => + { + while (!stop.IsCancellationRequested) + { + int[] seen = ReadAnsi(terminal); + if (!seen.SequenceEqual(darkAnsi) && !seen.SequenceEqual(lightAnsi)) + { + Interlocked.Increment(ref mixed); + } + } + }); + + var writer = Task.Run(() => + { + var toggle = false; + while (!stop.IsCancellationRequested) + { + terminal.Colors.ApplyTheme(toggle ? dark : light); + toggle = !toggle; + } + }); + + await Task.WhenAll(reader, writer); + + Assert.Equal(0, Volatile.Read(ref mixed)); + } + + /// + /// Reads the first eight ANSI colours through ONE snapshot. + /// + /// + /// Eight calls to the indexer would each take their own snapshot and could straddle a theme + /// change, which is the whole reason Take exists. + /// + private static int[] ReadAnsi(Terminal terminal) + { + ColorSnapshot snapshot = terminal.Colors.Take(); + var values = new int[8]; + for (var i = 0; i < values.Length; i++) + { + values[i] = snapshot[i]; + } + + return values; + } + + [Fact] + public void Take_ReturnsAViewThatDoesNotMoveAfterwards() + { + // The property that lets a renderer trust a snapshot for a whole frame. Writes copy on + // write for exactly this; mutating the live array in place would have been safe for the + // VALUE and still broken this. + var terminal = CreateTerminal(); + ColorSnapshot before = terminal.Colors.Take(); + int original = before[1]; + + terminal.Write("\u001b]4;1;#123456\u0007"); + + Assert.Equal(original, before[1]); + Assert.Equal(0x123456, terminal.Colors[1]); + Assert.Equal(0x123456, terminal.Colors.Take()[1]); + } + // ---- colour spec parsing ----------------------------------------------------------------- [Theory] diff --git a/src/XTerm.NET/Common/ColorPalette.cs b/src/XTerm.NET/Common/ColorPalette.cs index bc28299..5b69c83 100644 --- a/src/XTerm.NET/Common/ColorPalette.cs +++ b/src/XTerm.NET/Common/ColorPalette.cs @@ -1,3 +1,4 @@ +using System.Threading; using XTerm.Options; namespace XTerm.Common; @@ -22,15 +23,30 @@ public class ColorPalette public const int Size = 256; private readonly int[] defaults = new int[Size]; - private readonly int[] current = new int[Size]; + + /// Serialises writers against each other. Readers never take it. + private readonly object writeGate = new(); private int defaultForeground; private int defaultBackground; private int defaultCursor; - private int foregroundBacking; - private int backgroundBacking; - private int cursorBacking; + /// + /// The live colours. Replaced wholesale for bulk changes, mutated in place for single ones. + /// + /// + /// Read on a renderer's hot path -- once per cell -- so reads are lock free, and a lock here + /// would be a worse cure than the problem. What that costs is a discipline rather than a lock: + /// + /// SINGLE colour changes mutate the array or a scalar in place. An aligned int write is atomic, + /// so a reader sees the old value or the new one and never half of either. + /// + /// BULK changes -- ApplyTheme, ResetAllColors -- build a whole new snapshot and swap the + /// reference. Copying into the live array instead was the actual bug: Array.Copy is not atomic, + /// so a renderer could paint a frame with half the old theme and half the new one. A reference + /// swap has no half. + /// + private ColorSnapshot state; /// /// Initializes a palette with the built-in xterm defaults. @@ -46,7 +62,11 @@ public ColorPalette() /// public ColorPalette(ThemeOptions? theme) { - ApplyTheme(theme); + // Seeded inline rather than through ApplyTheme, so construction does not raise ColorChanged. + // Nothing can be subscribed yet, so the event went nowhere, but it made ApplyTheme's + // behaviour depend on when it was called. + SeedDefaults(theme); + state = NewSnapshot(); } /// @@ -57,17 +77,17 @@ public ColorPalette(ThemeOptions? theme) /// /// The current foreground colour, as 0xRRGGBB. /// - public int Foreground => foregroundBacking; + public int Foreground => Volatile.Read(ref state).Foreground; /// /// The current background colour, as 0xRRGGBB. /// - public int Background => backgroundBacking; + public int Background => Volatile.Read(ref state).Background; /// /// The current cursor colour, as 0xRRGGBB. /// - public int Cursor => cursorBacking; + public int Cursor => Volatile.Read(ref state).Cursor; /// /// Whether the background is light enough that a program should choose dark text. @@ -77,12 +97,31 @@ public ColorPalette(ThemeOptions? theme) /// consumer that wants it would otherwise write the same formula, and would be likely to write /// it as a plain average of the channels -- which calls pure blue light. /// - public bool IsLightBackground => Luma(Background) > 0.5; + public bool IsLightBackground => IsLight(Background); /// /// Gets the current colour for an index, as 0xRRGGBB. /// - public int this[int index] => current[Clamp(index)]; + public int this[int index] + { + get + { + ValidateIndex(index); + return Volatile.Read(ref state).Colors[index]; + } + } + + /// + /// Takes a coherent, immutable view of every colour at this instant. + /// + /// + /// What to use when reading MORE THAN ONE colour, which for a renderer painting a frame is + /// always. The individual properties on this class each read the current state separately, so + /// eight reads can straddle a theme change and return a mix -- measured at three and a half + /// million mixed reads per three seconds under a tight toggle, so this is routine rather than a + /// rare race. One snapshot cannot: it is a single reference, and nothing mutates it afterwards. + /// + public ColorSnapshot Take() => Volatile.Read(ref state); /// /// Replaces the defaults from a theme and discards any colours programs had set. @@ -94,12 +133,30 @@ public ColorPalette(ThemeOptions? theme) /// public void ApplyTheme(ThemeOptions? theme) { - SeedDefaults(theme); - Array.Copy(defaults, current, Size); - foregroundBacking = defaultForeground; - backgroundBacking = defaultBackground; - cursorBacking = defaultCursor; - ColorChanged?.Invoke(this, new ColorChangedEventArgs(ColorTarget.All, -1, 0)); + bool changed; + + lock (writeGate) + { + SeedDefaults(theme); + + var live = state; + changed = live.Foreground != defaultForeground + || live.Background != defaultBackground + || live.Cursor != defaultCursor + || !live.Colors.AsSpan().SequenceEqual(defaults); + + if (changed) + { + // One swap, so no reader ever sees a partly applied theme. + Volatile.Write(ref state, NewSnapshot()); + } + } + + // Raised outside the lock: a handler is consumer code and may call back in. + if (changed) + { + ColorChanged?.Invoke(this, new ColorChangedEventArgs(ColorTarget.All, -1, 0)); + } } /// @@ -107,37 +164,50 @@ public void ApplyTheme(ThemeOptions? theme) /// public void SetColor(int index, int rgb) { - index = Clamp(index); - if (current[index] == rgb) + ValidateIndex(index); + + lock (writeGate) { - return; + var live = state; + if (live.Colors[index] == rgb) + { + return; + } + + // Copy on write, rather than storing into the live array. An int store is atomic, so + // mutating in place would be safe for the VALUE -- but it would also change a snapshot + // somebody is already holding, and the whole point of handing one out is that it does + // not move underneath them. A palette is 1KB and colour changes are rare. + var colors = new int[Size]; + Array.Copy(live.Colors, colors, Size); + colors[index] = rgb; + Volatile.Write(ref state, new ColorSnapshot(colors, live.Foreground, live.Background, live.Cursor)); } - current[index] = rgb; ColorChanged?.Invoke(this, new ColorChangedEventArgs(ColorTarget.Indexed, index, rgb)); } /// /// Sets the foreground colour, as OSC 10 does. /// - public void SetForeground(int rgb) => Set(ref foregroundBacking, rgb, ColorTarget.Foreground); + public void SetForeground(int rgb) => Set(ColorTarget.Foreground, rgb); /// /// Sets the background colour, as OSC 11 does. /// - public void SetBackground(int rgb) => Set(ref backgroundBacking, rgb, ColorTarget.Background); + public void SetBackground(int rgb) => Set(ColorTarget.Background, rgb); /// /// Sets the cursor colour, as OSC 12 does. /// - public void SetCursor(int rgb) => Set(ref cursorBacking, rgb, ColorTarget.Cursor); + public void SetCursor(int rgb) => Set(ColorTarget.Cursor, rgb); /// /// Restores one indexed colour to its default, as OSC 104 with a parameter does. /// public void ResetColor(int index) { - index = Clamp(index); + ValidateIndex(index); SetColor(index, defaults[index]); } @@ -146,7 +216,19 @@ public void ResetColor(int index) /// public void ResetAllColors() { - Array.Copy(defaults, current, Size); + lock (writeGate) + { + var live = state; + if (live.Colors.AsSpan().SequenceEqual(defaults)) + { + return; + } + + var colors = new int[Size]; + Array.Copy(defaults, colors, Size); + Volatile.Write(ref state, new ColorSnapshot(colors, live.Foreground, live.Background, live.Cursor)); + } + ColorChanged?.Invoke(this, new ColorChangedEventArgs(ColorTarget.Indexed, -1, 0)); } @@ -165,6 +247,8 @@ public void ResetAllColors() /// public void ResetCursor() => SetCursor(defaultCursor); + internal static bool IsLight(int rgb) => Luma(rgb) > 0.5; + private static double Luma(int rgb) { var r = ((rgb >> 16) & 0xFF) / 255.0; @@ -173,19 +257,56 @@ private static double Luma(int rgb) return (0.299 * r) + (0.587 * g) + (0.114 * b); } - private static int Clamp(int index) => index < 0 ? 0 : index >= Size ? Size - 1 : index; + /// + /// Rejects an index outside the palette. + /// + /// + /// Throws rather than clamping. Clamping is the one response that produces a plausible wrong + /// answer: SetColor(999, ...) quietly rewrote entry 255, and the indexer answered for entry 0 + /// when asked about -1. A caller with an off-by-one got no signal and a corrupted palette. The + /// OSC path never sees this, because InputHandler range-checks before calling -- which is + /// exactly why it went unnoticed. + /// + private static void ValidateIndex(int index) + { + if (index < 0 || index >= Size) + { + throw new ArgumentOutOfRangeException( + nameof(index), index, $"Palette index must be between 0 and {Size - 1}."); + } + } - private void Set(ref int backing, int rgb, ColorTarget target) + private void Set(ColorTarget target, int rgb) { - if (backing == rgb) + lock (writeGate) { - return; + var live = state; + int foreground = live.Foreground, background = live.Background, cursor = live.Cursor; + + switch (target) + { + case ColorTarget.Foreground when foreground != rgb: foreground = rgb; break; + case ColorTarget.Background when background != rgb: background = rgb; break; + case ColorTarget.Cursor when cursor != rgb: cursor = rgb; break; + default: return; + } + + Volatile.Write(ref state, new ColorSnapshot(live.Colors, foreground, background, cursor)); } - backing = rgb; ColorChanged?.Invoke(this, new ColorChangedEventArgs(target, -1, rgb)); } + /// + /// Builds a snapshot holding the current defaults. + /// + private ColorSnapshot NewSnapshot() + { + var colors = new int[Size]; + Array.Copy(defaults, colors, Size); + return new ColorSnapshot(colors, defaultForeground, defaultBackground, defaultCursor); + } + private void SeedDefaults(ThemeOptions? theme) { // 0-15: the xterm ANSI defaults, each overridable by the theme. @@ -239,6 +360,57 @@ private void SeedDefaults(ThemeOptions? theme) defaultBackground = ColorSpec.TryParse(theme?.Background, out var bg) ? bg : 0x000000; defaultCursor = ColorSpec.TryParse(theme?.Cursor, out var cur) ? cur : 0xFFFFFF; } + + +} + +/// +/// An immutable view of every terminal colour at one instant. +/// +/// +/// Handed out by and never modified afterwards, so a renderer can +/// paint a whole frame from one and know the colours belong to each other. +/// +public sealed class ColorSnapshot +{ + private readonly int[] colors; + + internal ColorSnapshot(int[] colors, int foreground, int background, int cursor) + { + this.colors = colors; + Foreground = foreground; + Background = background; + Cursor = cursor; + } + + /// Gets the foreground colour, as 0xRRGGBB. + public int Foreground { get; } + + /// Gets the background colour, as 0xRRGGBB. + public int Background { get; } + + /// Gets the cursor colour, as 0xRRGGBB. + public int Cursor { get; } + + /// Gets whether the background is light enough that dark text belongs on it. + public bool IsLightBackground => ColorPalette.IsLight(Background); + + /// Gets the colour for an index, as 0xRRGGBB. + public int this[int index] + { + get + { + if (index < 0 || index >= ColorPalette.Size) + { + throw new ArgumentOutOfRangeException( + nameof(index), index, $"Palette index must be between 0 and {ColorPalette.Size - 1}."); + } + + return colors[index]; + } + } + + internal int[] Colors => colors; } ///