-
-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: inspect/enrich event in crashpad backend #843
feat: inspect/enrich event in crashpad backend #843
Conversation
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #843 +/- ##
==========================================
+ Coverage 82.08% 82.57% +0.48%
==========================================
Files 52 53 +1
Lines 7141 7352 +211
Branches 1149 1185 +36
==========================================
+ Hits 5862 6071 +209
+ Misses 1181 1172 -9
- Partials 98 109 +11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -183,6 +191,7 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context) | |||
} else { | |||
SENTRY_TRACE("event was discarded"); | |||
} | |||
sentry_value_decref(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there is a leak and double-free here, with the one in crashpad_backend_free
In particular, setting data->crash_event
above does not decref
any value that was previously there, and it transfers ownership, so it should probably incref before doing so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, but wouldn't the preferable option be to only decref
the event
in the "discarded" path and leave the reference count given that we only transfer ownership from one ref to the other (and event
isn't accessed any longer)? Or do you mean in case someone access event
later on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In particular, setting
data->crash_event
above does notdecref
any value that was previously there, and it transfers ownership, so it should probably incref before doing so.
@markushi asked the same thing in the last PR. We can defensively decref
the data->crash_event
, but there is no allocation happening before this at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all very academic, since you are in the crash path here, so you will not ever call into this function again. Though I believe its still worth doing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I understand. Even if the effect of fixing this is inconsequential to the execution of the crash-handler, it should still be correct as documentation for future code additions. I just got rid of the local event
and only use data->crash_event
in the crash-handler (which is now defensifely decref
ed).
Since no one commented on my suggestion here: #840 (comment), here is the PR that implements that :P