fix: check for null value returned from pthread_mach_thread_np #3443
Conversation
| const auto activeThreadID = | ||
| [transaction.trace.transactionContext sentry_threadInfo].threadId ?: @(-1); | ||
| payload[@"transaction"] = @{ | ||
| @"id" : transaction.eventId.sentryIdString, | ||
| @"trace_id" : transaction.trace.traceId.sentryIdString, | ||
| @"name" : transaction.transaction, | ||
| @"active_thread_id" : [transaction.trace.transactionContext sentry_threadInfo].threadId | ||
| @"active_thread_id" : activeThreadID |
There was a problem hiding this comment.
I'm not sure what the right thing to put here would be.
There was a problem hiding this comment.
Do you mean if the thread ID is NULL? If yes, couldn't we just not set the active_thread_id?
There was a problem hiding this comment.
maybe; i'm not sure what ramifications that would have in the backend
There was a problem hiding this comment.
@phacops Please let us know how we would proceed in this case. Maybe we just choose not to send the profile at all.
| if (thread == (mach_port_t)NULL) { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
I don't think we actually need this, the crash was actually happening when current() is dereferenced (see the callsites in this diff)
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3443 +/- ##
=============================================
+ Coverage 88.967% 89.016% +0.049%
=============================================
Files 525 525
Lines 56577 56630 +53
Branches 20357 20390 +33
=============================================
+ Hits 50335 50410 +75
+ Misses 5317 5300 -17
+ Partials 925 920 -5
... and 26 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2af280d | 1246.22 ms | 1253.10 ms | 6.88 ms |
| bb5dc7d | 1240.44 ms | 1266.45 ms | 26.01 ms |
| 2b4a663 | 1220.55 ms | 1249.98 ms | 29.43 ms |
| 407ff99 | 1250.10 ms | 1269.78 ms | 19.67 ms |
| e2abb0d | 1235.08 ms | 1257.00 ms | 21.92 ms |
| 230ba67 | 1230.18 ms | 1252.32 ms | 22.13 ms |
| 45d3ca5 | 1248.27 ms | 1255.48 ms | 7.21 ms |
| 904d7fa | 1225.73 ms | 1249.22 ms | 23.49 ms |
| 881a955 | 1222.16 ms | 1237.22 ms | 15.06 ms |
| 39b1c35 | 1235.90 ms | 1257.37 ms | 21.47 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2af280d | 20.76 KiB | 435.22 KiB | 414.46 KiB |
| bb5dc7d | 22.85 KiB | 412.98 KiB | 390.13 KiB |
| 2b4a663 | 20.76 KiB | 436.65 KiB | 415.89 KiB |
| 407ff99 | 20.76 KiB | 427.87 KiB | 407.10 KiB |
| e2abb0d | 20.76 KiB | 434.72 KiB | 413.96 KiB |
| 230ba67 | 20.76 KiB | 427.35 KiB | 406.59 KiB |
| 45d3ca5 | 20.76 KiB | 427.54 KiB | 406.78 KiB |
| 904d7fa | 20.76 KiB | 432.87 KiB | 412.11 KiB |
| 881a955 | 22.85 KiB | 407.63 KiB | 384.78 KiB |
| 39b1c35 | 22.85 KiB | 408.88 KiB | 386.03 KiB |
There was a problem hiding this comment.
What's missing to finish this PR apart from tests, @armcknight?
| const auto activeThreadID = | ||
| [transaction.trace.transactionContext sentry_threadInfo].threadId ?: @(-1); | ||
| payload[@"transaction"] = @{ | ||
| @"id" : transaction.eventId.sentryIdString, | ||
| @"trace_id" : transaction.trace.traceId.sentryIdString, | ||
| @"name" : transaction.transaction, | ||
| @"active_thread_id" : [transaction.trace.transactionContext sentry_threadInfo].threadId | ||
| @"active_thread_id" : activeThreadID |
There was a problem hiding this comment.
Do you mean if the thread ID is NULL? If yes, couldn't we just not set the active_thread_id?
The customer reported that this branch doesn't resolve the crash. |
|
#3520 builds on this by adding more stringent and additional checks in other spots, so I'll close this in favor of moving that one forward. |
We didn't completely fix the problem reported in #3354 with #3364. This changes the callsites to check for
NULLbefore dereferencing the pointer.I checked the sources and it does appear that
NULLis a possible return value frompthread_mach_thread_np: https://github.com/apple/darwin-libpthread/blob/2b46cbcc56ba33791296cd9714b2c90dae185ec7/src/pthread.c#L931-L937 and for_pthread_is_valid, https://opensource.apple.com/source/libpthread/libpthread-416.11.1/src/internal.h.