fix: deep-copy user mutated attributes #1148
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| if event.User.Data != nil { | ||
| eventToBuffer.User.Data = deepCopyMapStringString(event.User.Data) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1148 +/- ##
==========================================
- Coverage 86.85% 85.83% -1.02%
==========================================
Files 62 63 +1
Lines 6092 6184 +92
==========================================
+ Hits 5291 5308 +17
- Misses 587 656 +69
- Partials 214 220 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| k := iter.Key() | ||
| val := iter.Value().Interface() | ||
| newVal := deepCopyValue(val) | ||
| newMap.SetMapIndex(k, reflect.ValueOf(newVal)) |
There was a problem hiding this comment.
Bug: Nil values in nested maps/slices cause data loss or panic
When deepCopyValue returns nil for a nil value in a nested structure, reflect.ValueOf(nil) produces an invalid reflect.Value. For maps, calling SetMapIndex with an invalid Value silently deletes the key, causing data loss. For slices and arrays, calling Set with an invalid Value causes a panic. This affects user-provided Extra and Context data containing nested collections with nil values.
Additional Locations (2)
| clone.breadcrumbs = make([]*Breadcrumb, len(scope.breadcrumbs)) | ||
| copy(clone.breadcrumbs, scope.breadcrumbs) | ||
| clone.breadcrumbs = make([]*Breadcrumb, 0, len(scope.breadcrumbs)) | ||
| for _, b := range scope.breadcrumbs { | ||
| clone.breadcrumbs = append(clone.breadcrumbs, deepCopyBreadcrumb(b)) | ||
| } | ||
| clone.attachments = make([]*Attachment, len(scope.attachments)) | ||
| copy(clone.attachments, scope.attachments) | ||
| for key, value := range scope.tags { | ||
| clone.tags[key] = value | ||
| } | ||
| for key, value := range scope.contexts { | ||
| clone.contexts[key] = cloneContext(value) | ||
| } | ||
| for key, value := range scope.extra { | ||
| clone.extra[key] = value |
There was a problem hiding this comment.
Correct me if I'm wrong, but:
this was here already and was not causing problems, and it's a separate code path from the buffer/transport, so I don't think this should be changed at all.
I assume this was deliberately done this way because scope forking happens frequently and so you wouldn't want to do an expensive deep copy.
| // a proper deep copy: if some context values are pointer types (e.g. maps), | ||
| // they won't be properly copied. | ||
| func cloneContext(c Context) Context { | ||
| res := make(Context, len(c)) |
| if event.User.Data != nil { | ||
| eventToBuffer.User.Data = deepCopyMapStringString(event.User.Data) | ||
| } | ||
|
|
There was a problem hiding this comment.
Nested in Event there are other things we might care about, for example a Span can have Data (and apparently tags and extra), same about Log attributes, etc. Do we care about those?
|
...or we can serialize it up front, and then send them to telemetry buffer after serialization? I would assume doing deep copy would inflict in more RAM usage than just serialize it up front. |
I was weighting both approaches, but i think that pre-serialization has more drawbacks:
That is a valid concern but we only keep the copy around till we flush the event (short lifespan) and other SDKs already copy some event data. |
|
|
||
| if client.telemetryBuffer != nil { | ||
| if !client.telemetryBuffer.Add(event) { | ||
| eventToBuffer := *event |
There was a problem hiding this comment.
Bug: The Event.Tags map is not deep-copied before being passed to the telemetry buffer, creating a risk of a concurrent map access panic if the user mutates it.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
When an event is captured with telemetry enabled, several of its fields are deep-copied to prevent data races during background serialization. However, the Event.Tags map, which is a user-mutable field, is not being deep-copied. If a user modifies the event.Tags map after calling CaptureEvent(), a concurrent map access panic will occur when the background worker attempts to serialize the event for telemetry. This omission is inconsistent with the handling of other mutable fields like Extra and Contexts.
💡 Suggested Fix
In client.go, before adding the event to the telemetry buffer, perform a deep copy of the event.Tags map, similar to how other mutable fields are handled. For example: if event.Tags != nil { eventToBuffer.Tags = deepCopyMapStringString(event.Tags) }.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: client.go#L766
Potential issue: When an event is captured with telemetry enabled, several of its fields
are deep-copied to prevent data races during background serialization. However, the
`Event.Tags` map, which is a user-mutable field, is not being deep-copied. If a user
modifies the `event.Tags` map after calling `CaptureEvent()`, a concurrent map access
panic will occur when the background worker attempts to serialize the event for
telemetry. This omission is inconsistent with the handling of other mutable fields like
`Extra` and `Contexts`.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8279488
| } | ||
| if event.User.Data != nil { | ||
| eventToBuffer.User.Data = deepCopyMapStringString(event.User.Data) | ||
| } |
There was a problem hiding this comment.
Tags field missing deep copy in telemetry buffer
High Severity
The Tags field is not deep copied when buffering events for background serialization, while other map[string]string fields like Modules and User.Data are. Since eventToBuffer := *event creates a shallow copy, eventToBuffer.Tags still references the same underlying map as event.Tags. If a user mutates the tags map after the event is queued, this could cause a race condition or panic during serialization in the background worker.
Description
Since the addition of Telemetry Buffers moves the serialization to a background worker, user provided attributes that can be mutated should be deep copied, to avoid panics during serialization.