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

read/macho: fix data for segments in dyld caches #673

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

philipc
Copy link
Contributor

@philipc philipc commented Apr 26, 2024

Note that this is only for operations on the segment itself. Sections within the segment were already using the correct data.

@philipc
Copy link
Contributor Author

philipc commented Apr 26, 2024

@mstange Can you review this, and do you know if the following code needs fixing too? Edit: I've fixed this too now.

.relocations(self.file.endian, self.file.data)

@mstange
Copy link
Contributor

mstange commented Apr 26, 2024

I'll take a look. I'm a bit surprised because I thought I was using segment data for the assembly view, here: https://github.com/mstange/samply/blob/6ec2c01f8e4f30c570321b8516fba975e62ab135/samply-symbols/src/binary_image.rs#L279-L280

@philipc
Copy link
Contributor Author

philipc commented Apr 27, 2024

The code looks wrong to me, but I haven't tested it. I'll see if I can put together a test.

@philipc
Copy link
Contributor Author

philipc commented Apr 29, 2024

I obtained a login to a macOS 13.6.6 system. I changed data_and_offset_for_address to also return the index of the subcache, and added an assert in parse_dyld_cache_image to check that the image and segment had the same subcache index. I then ran objdump on the cache, and the assert never triggered. Conclusion: I need a different macOS version in order to be able to test this.

@mstange
Copy link
Contributor

mstange commented Apr 29, 2024

I then ran objdump on the cache, and the assert never triggered.

I see the same on macOS 14.4.1. So maybe what I observed in the original commit on macOS 12.0.1 no longer happens:

For example, on macOS 12.0.1, the image for libsystem_malloc.dylib for the
arm64e architecture has its __TEXT segment in the root cache and the
__LINKEDIT segment in the .1 subcache - there's a single __LINKEDIT
segment which is shared between all images across both files. The
remaining libsystem_malloc.dylib segments are in the same file as the
__TEXT segment.

I've sent you an email with the macOS 12.0.1 arm64e dyld shared cache.

@mstange
Copy link
Contributor

mstange commented Apr 29, 2024

Ok, so I think the one case where the segment data would have been wrong is if you had asked for the data of the __LINKEDIT segment on a cache from macOS 12, for any image that's stored in the root cache.
I never noticed anything wrong because I only look at the data of the __TEXT segment, which is always located in the same subcache as the image (almost by definition, because the load commands are always at the start of the __TEXT segment, IIRC).

Anyway, I think your fix makes sense.

Use the correct subcache for operations on the segment itself.
Individual section data was already using the correct subcache.

This also does the change for section relocations, but they shouldn't
be present in the dyld cache.
@philipc philipc merged commit 74c757c into gimli-rs:master Apr 30, 2024
10 checks passed
@philipc philipc deleted the macho-segment-data branch April 30, 2024 04:46
@philipc
Copy link
Contributor Author

philipc commented Apr 30, 2024

Thanks for looking into this, and for the email. I did some manual testing to check that this had an effect.

mcbegamerxx954 pushed a commit to mcbegamerxx954/object that referenced this pull request Jun 15, 2024
Use the correct subcache for operations on the segment itself.
Individual section data was already using the correct subcache.

This also does the change for section relocations, but they shouldn't
be present in the dyld cache.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants