From cf39736f87613195aa8a2d7304ee20d7d9eeaa47 Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Fri, 28 Jul 2023 10:35:10 -0400 Subject: [PATCH] log/slog: don't panic on aliased Record If the shared slice in a copied is modified, make a copy of it and insert an attribute that warns of the bug. Previously, we panicked, and panics in logging code should be avoided. Change-Id: I24e9b0bf5c8cd09cf733e7dae8a82d025ef214e5 Reviewed-on: https://go-review.googlesource.com/c/go/+/513760 Run-TryBot: Jonathan Amsterdam Reviewed-by: Alan Donovan Reviewed-by: Ian Cottrell TryBot-Result: Gopher Robot --- src/log/slog/record.go | 4 +++- src/log/slog/record_test.go | 11 +++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/log/slog/record.go b/src/log/slog/record.go index 67b76f34e1..82acc7ac7b 100644 --- a/src/log/slog/record.go +++ b/src/log/slog/record.go @@ -109,7 +109,9 @@ func (r *Record) AddAttrs(attrs ...Attr) { if cap(r.back) > len(r.back) { end := r.back[:len(r.back)+1][len(r.back)] if !end.isEmpty() { - panic("copies of a slog.Record were both modified") + // Don't panic; copy and muddle through. + r.back = slices.Clip(r.back) + r.back = append(r.back, String("!BUG", "AddAttrs unsafely called on copy of Record made without using Record.Clone")) } } ne := countEmptyGroups(attrs[i:]) diff --git a/src/log/slog/record_test.go b/src/log/slog/record_test.go index 15d9330a85..931ab66041 100644 --- a/src/log/slog/record_test.go +++ b/src/log/slog/record_test.go @@ -96,12 +96,15 @@ func TestAliasingAndClone(t *testing.T) { r1.back = b // Make a copy that shares state. r2 := r1 - // Adding to both should panic. + // Adding to both should insert a special Attr in the second. + r1AttrsBefore := attrsSlice(r1) r1.AddAttrs(Int("p", 0)) - if !panics(func() { r2.AddAttrs(Int("p", 1)) }) { - t.Error("expected panic") - } + r2.AddAttrs(Int("p", 1)) + check(r1, append(slices.Clip(r1AttrsBefore), Int("p", 0))) r1Attrs := attrsSlice(r1) + check(r2, append(slices.Clip(r1AttrsBefore), + String("!BUG", "AddAttrs unsafely called on copy of Record made without using Record.Clone"), Int("p", 1))) + // Adding to a clone is fine. r2 = r1.Clone() check(r2, r1Attrs) -- 2.48.1