Skip to content

Commit fea684f

Browse files
blog: Stop mutating caller-owned attr slices (#8847)
Error, AuditError, and AuditInfo appended internal attrs (error, audit) directly to the caller's variadic slice. When a caller passes a slice with spare capacity, that append writes into the caller's backing array, leaking a stray attr into any later use of the slice. Clip the slice first so the append always allocates. (ContextWith was given the equivalent fix during #8606 review, but these methods were missed.) This PR was generated as part of an audit of #8606 using Claude Fable 5.
1 parent 92a55b7 commit fea684f

2 files changed

Lines changed: 56 additions & 3 deletions

File tree

blog/logger.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"log/slog"
1616
"log/syslog"
1717
"os"
18+
"slices"
1819

1920
"github.com/letsencrypt/boulder/core"
2021
)
@@ -91,7 +92,7 @@ func (l *logger) Error(ctx context.Context, msg string, err error, attrs ...slog
9192
// We attach these attrs to the context, rather than passing them directly to
9293
// LogAttrs, to ensure that they come last in the log line. See
9394
// contextHandler.Handle() for more information.
94-
ctx = ContextWith(ctx, append(attrs, Error(err))...)
95+
ctx = ContextWith(ctx, append(slices.Clip(attrs), Error(err))...)
9596
l.inner.LogAttrs(ctx, slog.LevelError, msg)
9697
}
9798

@@ -117,13 +118,13 @@ func (l *logger) Debug(ctx context.Context, msg string, attrs ...slog.Attr) {
117118
// level and with the audit tag. The error will be included in the attrs under
118119
// the key "error".
119120
func (l *logger) AuditError(ctx context.Context, msg string, err error, attrs ...slog.Attr) {
120-
ctx = ContextWith(ctx, append(attrs, auditAttr, Error(err))...)
121+
ctx = ContextWith(ctx, append(slices.Clip(attrs), auditAttr, Error(err))...)
121122
l.inner.LogAttrs(ctx, slog.LevelError, msg)
122123
}
123124

124125
// AuditInfo logs the given message and other key-value pairs at info level and
125126
// with the audit tag.
126127
func (l *logger) AuditInfo(ctx context.Context, msg string, attrs ...slog.Attr) {
127-
ctx = ContextWith(ctx, append(attrs, auditAttr)...)
128+
ctx = ContextWith(ctx, append(slices.Clip(attrs), auditAttr)...)
128129
l.inner.LogAttrs(ctx, slog.LevelInfo, msg)
129130
}

blog/logger_test.go

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,3 +188,55 @@ func TestLoggerChecksum(t *testing.T) {
188188
t.Errorf("checksum prefix %q does not match LogLineChecksum of remainder %q", parts[0], want)
189189
}
190190
}
191+
192+
func TestLoggerDoesNotMutateCallerAttrs(t *testing.T) {
193+
t.Parallel()
194+
195+
// These methods append internal attrs (error, audit) to the caller's
196+
// variadic attr slice. If that append reuses the caller's backing array,
197+
// the caller's slice is corrupted for any later use.
198+
testCases := []struct {
199+
name string
200+
log func(l Logger, attrs ...slog.Attr)
201+
}{
202+
{
203+
name: "Error",
204+
log: func(l Logger, attrs ...slog.Attr) {
205+
l.Error(context.Background(), "oops", errors.New("boom"), attrs...)
206+
},
207+
},
208+
{
209+
name: "AuditError",
210+
log: func(l Logger, attrs ...slog.Attr) {
211+
l.AuditError(context.Background(), "oops", errors.New("boom"), attrs...)
212+
},
213+
},
214+
{
215+
name: "AuditInfo",
216+
log: func(l Logger, attrs ...slog.Attr) {
217+
l.AuditInfo(context.Background(), "note", attrs...)
218+
},
219+
},
220+
}
221+
222+
for _, tc := range testCases {
223+
t.Run(tc.name, func(t *testing.T) {
224+
t.Parallel()
225+
226+
attrs := make([]slog.Attr, 0, 4)
227+
attrs = append(attrs, slog.String("k1", "v1"))
228+
229+
tc.log(NewMock(), attrs...)
230+
231+
if len(attrs) != 1 || attrs[0].String() != "k1=v1" {
232+
t.Errorf("caller's attr slice was mutated: %v", attrs)
233+
}
234+
leaked := attrs[:cap(attrs)]
235+
for i := 1; i < len(leaked); i++ {
236+
if !leaked[i].Equal(slog.Attr{}) {
237+
t.Errorf("caller's backing array index %d contains leaked attr %v", i, leaked[i])
238+
}
239+
}
240+
})
241+
}
242+
}

0 commit comments

Comments
 (0)