Skip to content
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

[SharedCache] Apply objc_msgSend call type overrides in more places #6436

Closed

Conversation

bdash
Copy link
Contributor

@bdash bdash commented Feb 20, 2025

fixObjCCallTypes is updated to handle tail calls as well as regular calls.

Additionally, if the selector address is not valid it now looks for the sel_ symbols that DSCObjCProcessor::ReadMethodList adds for selectors whose names reside in regions that are not yet mapped. This allows call type overrides to be applied even when a selector's name is defined in a different image. This is the common case in recent macOS shared caches at least.

@bdash
Copy link
Contributor Author

bdash commented Feb 20, 2025

This works, but Binary Ninja seems to get confused about the state of some functions.

In some cases the override doesn’t appear to take. When I scroll the function containing the override to the right place in the view I see a little progress spinner appear at the top of the view, where the “Reanalyze function” arrow would usually be. It just spins and spins. If I right-click on one of the objc_msgSend calls that has had its type overridden and select “Override Call Type…” it shows the expected overridden type. If I hit Accept the call (and others in the same function) all update to reflect the overrides.

Is this an error in fixObjCCallTypes, or a bug elsewhere?

@bdash bdash marked this pull request as draft February 20, 2025 00:09
@bdash bdash force-pushed the dsc-objc-msgsend-call-type-overrides branch from 1d8ba2d to 1c531f6 Compare March 5, 2025 06:08
@bdash bdash force-pushed the dsc-objc-msgsend-call-type-overrides branch 3 times, most recently from 38fbc9a to 5457e3a Compare April 3, 2025 13:43
@bdash bdash marked this pull request as ready for review April 3, 2025 13:43
@fuzyll fuzyll added the File Format: SharedCache Issue with the dyld_shared_cache plugin label Apr 3, 2025
@fuzyll fuzyll added this to the Gallifrey milestone Apr 3, 2025
`fixObjCCallTypes` is updated to handle tail calls as well as regular
calls.

Additionally, if the selector address is not valid it now looks for the
`sel_` symbols that `DSCObjCProcessor::ReadMethodList` adds for
selectors whose names reside in regions that are not yet mapped. This
allows call type overrides to be applied even when a selector's name is
defined in a different image.
@bdash bdash force-pushed the dsc-objc-msgsend-call-type-overrides branch from 5457e3a to d255abe Compare April 7, 2025 06:12
@plafosse plafosse modified the milestones: Gallifrey, H Apr 7, 2025
@emesare
Copy link
Member

emesare commented Apr 8, 2025

Can you give an example of a function that is improved, just so i can verify to make sure the refactor did not mess this up

@bdash
Copy link
Contributor Author

bdash commented Apr 8, 2025

In MediaLibrary.framework, look for:

-[MLMediaLibrary init]
-[MLMediaLibrary outboundRequestQueue]
-[MLMediaLibrary outboundRequestCondition]
-[MLPMRContext cppInstrument:]
-[MLPMRContext applicationName]
-[MLPMRContext logPath]

They each have a tail call to objc_msgSend that is displayed as return _objc_msgSend() __tailcall with no arguments without this change.

@emesare
Copy link
Member

emesare commented Apr 8, 2025

Before:
image

After:
image

LGTM

@emesare emesare modified the milestones: H, Gallifrey Apr 8, 2025
@emesare
Copy link
Member

emesare commented Apr 8, 2025

Added with a6c5c22

On dev with >7183

@emesare emesare closed this Apr 8, 2025
@bdash bdash deleted the dsc-objc-msgsend-call-type-overrides branch April 8, 2025 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
File Format: SharedCache Issue with the dyld_shared_cache plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants