fix: deep-copy user mutated attributes #1148
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
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}
| 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)) |
client.go
Outdated
| 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. |
| } | ||
|
|
||
| for key, value := range scope.extra { | ||
| event.Extra[key] = value | ||
| event.Extra[key] = deepCopyValue(value) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: Breadcrumbs are not deep-copied when added to the scope. If a user mutates a breadcrumb's Data map after adding it, a race condition can occur during serialization.
Severity: CRITICAL
🔍 Detailed Analysis
The AddBreadcrumb function appends breadcrumbs by reference, but breadcrumbs can contain a mutable Data map. If a user adds a breadcrumb and later modifies its Data map, this can create a race condition between the user's code and the background serialization worker that reads the map. This can lead to a runtime panic from concurrent map access. This behavior is inconsistent with the pull request's goal of deep-copying other mutable user-provided attributes like extra and context to prevent this exact issue.
💡 Suggested Fix
Deep-copy the Breadcrumb.Data map when a breadcrumb is added to the scope via AddBreadcrumb. This would align the handling of breadcrumbs with that of extra and context data, preventing potential race conditions during background serialization.
🤖 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: scope.go#L443-L449
Potential issue: The `AddBreadcrumb` function appends breadcrumbs by reference, but
breadcrumbs can contain a mutable `Data` map. If a user adds a breadcrumb and later
modifies its `Data` map, this can create a race condition between the user's code and
the background serialization worker that reads the map. This can lead to a runtime panic
from concurrent map access. This behavior is inconsistent with the pull request's goal
of deep-copying other mutable user-provided attributes like `extra` and `context` to
prevent this exact issue.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8391684
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.