-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[Crashlytics] Conforming to Mach IPC security restrictions #15612
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
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
8030fd5 to
c35e403
Compare
d22c92e to
791dc5d
Compare
2dda7ac to
c7a16af
Compare
|
/gemini review |
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.
Code Review
This pull request correctly updates the Mach exception handling to conform to newer security restrictions by using EXCEPTION_IDENTITY_PROTECTED. The changes to use task identity tokens and thread IDs are well-implemented. However, I've found a couple of critical issues in the new FIRCLSCrashedThreadLookup function and its usage. One is a use-after-free bug due to premature deallocation of the crashed thread's port, and another is the use of an uninitialized variable. I've provided detailed comments and suggestions to fix these issues. Once these are addressed, the changes should be solid.
7b56d58 to
7c052fe
Compare
|
/gemini review |
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.
Code Review
This pull request updates the Mach exception handling to conform to newer security restrictions by using EXCEPTION_IDENTITY_PROTECTED. This changes the exception message format, and the code has been updated to handle this by getting the task port from a token and looking up the thread port from a thread ID. The changes are well-implemented, but I've identified two resource leaks related to Mach port handling that should be addressed. Otherwise, the changes look good.
|
/gemini review |
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.
Code Review
This pull request correctly updates the Mach exception handling to conform to Apple's new Mach IPC security restrictions by using EXCEPTION_IDENTITY_PROTECTED. The changes properly adapt the exception message handling, including looking up the task port from a token and finding the crashed thread by its ID. The resource management for the acquired Mach ports also appears to be handled correctly. I have a minor suggestion to improve code clarity by renaming a struct field to avoid confusion with a type name.
|
The failures look like flakes in the context manager tests. That class has some async dispatches, so it is likely those not resolving due to background priority. |
Related issue: #15393
Tested on:
Note: macOS10.5 and 11 does not support identity protection so it fallback to default behavior, addressed in the later pr
Change exception behaviors from default to identity protected
Context: https://developer.apple.com/documentation/xcode/conforming-to-mach-ipc-security-restrictions