mcp: fix a race condition in logging
Reading the buffer outside of a critical section can lead to data corruption.
This commit is contained in:
parent
52e03de6c8
commit
6b75899fd7
1 changed files with 6 additions and 3 deletions
|
|
@ -10,6 +10,7 @@ import (
|
|||
"context"
|
||||
"encoding/json"
|
||||
"log/slog"
|
||||
"slices"
|
||||
"sync"
|
||||
"time"
|
||||
)
|
||||
|
|
@ -165,8 +166,7 @@ func (h *LoggingHandler) Handle(ctx context.Context, r slog.Record) error {
|
|||
|
||||
func (h *LoggingHandler) handle(ctx context.Context, r slog.Record) error {
|
||||
// Observe the rate limit.
|
||||
// TODO(jba): use golang.org/x/time/rate. (We can't here because it would require adding
|
||||
// golang.org/x/time to the go.mod file.)
|
||||
// TODO(jba): use golang.org/x/time/rate.
|
||||
h.mu.Lock()
|
||||
skip := time.Since(h.lastMessageSent) < h.opts.MinInterval
|
||||
h.mu.Unlock()
|
||||
|
|
@ -175,6 +175,7 @@ func (h *LoggingHandler) handle(ctx context.Context, r slog.Record) error {
|
|||
}
|
||||
|
||||
var err error
|
||||
var data json.RawMessage
|
||||
// Make the buffer reset atomic with the record write.
|
||||
// We are careful here in the unlikely event that the handler panics.
|
||||
// We don't want to hold the lock for the entire function, because Notify is
|
||||
|
|
@ -185,6 +186,8 @@ func (h *LoggingHandler) handle(ctx context.Context, r slog.Record) error {
|
|||
defer h.mu.Unlock()
|
||||
h.buf.Reset()
|
||||
err = h.handler.Handle(ctx, r)
|
||||
// Clone the buffer as Bytes() references the internal buffer.
|
||||
data = json.RawMessage(slices.Clone(h.buf.Bytes()))
|
||||
}()
|
||||
if err != nil {
|
||||
return err
|
||||
|
|
@ -197,7 +200,7 @@ func (h *LoggingHandler) handle(ctx context.Context, r slog.Record) error {
|
|||
params := &LoggingMessageParams{
|
||||
Logger: h.opts.LoggerName,
|
||||
Level: slogLevelToMCP(r.Level),
|
||||
Data: json.RawMessage(h.buf.Bytes()),
|
||||
Data: data,
|
||||
}
|
||||
// We pass the argument context to Notify, even though slog.Handler.Handle's
|
||||
// documentation says not to.
|
||||
|
|
|
|||
Loading…
Reference in a new issue