Check MACH_PORT_VALID on return from pthread_mach_thread_np

[indragiek]

📜 Description

pthread_mach_thread_np can return invalid values so check for those and return nullptr from ThreadHandle. That means changing the call sites for ThreadHandle::current() to check for a null pointer value.

💡 Motivation and Context

We don't have a solid repro for #3354 but this is one of the potential causes. In any case it doesn't hurt to add the check.

💚 How did you test it?

Run tests

📝 Checklist

You have to check all boxes before merging:

• I reviewed the submitted code.
• I added tests to verify the changes.
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• I updated the docs if needed.
• Review from the native team if needed.
• No breaking change or entry added to the changelog.
• No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

[philipphofmann]

@indragiek, good catch 👍 . CI is broken, and it would be great to add or change some tests for these changes if possible.

[philipphofmann]

h: If I'm not mistaken, we have to check for a potential nullptr of this method here

sentry-cocoa/Sources/Sentry/SentryBacktrace.cpp

Lines 159 to 160 in 48e8c2e

	const auto depth = backtrace(*thread, *pair.second, addresses, stackBounds,
	&reachedEndOfStack, kMaxBacktraceDepth, 0);

or does backtrace handle nullptr correctly?

[armcknight]

Good catch, doesn't look like there's a check for nullptr before this dereference.

[armcknight]

Reading the logic again in SentryBacktrace.cpp which contains the callsite for the function we've modified is a bit unclear to me, given the name allExcludingCurrent. Reading it starting at the loop, it looks like we're going to do work on all the other threads except the current one, which is true, but we do still have to retain the current thread in order to know what thread we're running on, and the callsite tells me we threw out that info when we actually didn't.

TL;DR I would propose we rename allExcludingCurrent to currentAndOtherThreads.

[armcknight]

Good catch, doesn't look like there's a check for nullptr before this dereference.