feat: install name tool style macho writing #509
Conversation
|
Hello, thanks for opening the PR! Before we go further, I think it's important to first understand what parts of this PR enable functionality to write install_name_tool with goblin. If i'm reading this correctly, most of the code does not appear to require being in goblin, and almost everything in src/mach/writer.rs (a new file) could be either in your source tree (if you use goblin) or a crate that depends on goblin and implements this functionality. There are some other odds and ends that look useful, e.g., the size changes, but I'd first like to narrow down if there's any api changs/new apis you need in order to write src/mach/writer.rs externally. In general, glancing at what it does, it seems very niche/oriented specifically for that usecase, which is generally something I like to avoid in goblin, as it's more a general purpose library, and when we add new apis like that we/I have to maintain them forever basically. That being said, there's nothing necessarily wrong with adding new apis, but in general, I would prefer to first identify if this can just be an external crate (e.g., https://github.com/m4b/faerie, https://github.com/m4b/bingrep, are examples of external crates/tools that use goblin as client), and if so, I'd encourage to just publish as an external crate. If there are some api changes that are needed to allow that usecase, then i'm definitely open to changes that would unlock that potential :). Thanks, and let me know if you have any questions! |
m4b
left a comment
m4b
left a comment
There was a problem hiding this comment.
added some comments ont he load command and fat stuff, which i think looks reasonable. as i mentioned though, I'd like to understand what api portions are required to write what's in writer.rs, instead of maintaining all of that in goblin.
| } | ||
|
|
||
| pub const SIZEOF_DYLIB_COMMAND: usize = 20; | ||
| pub const SIZEOF_DYLIB_COMMAND: usize = 24; |
There was a problem hiding this comment.
I need to understand why this changed (and perhaps why a test didn't fail when it did?)
| pub const FAT_MAGIC_64: u32 = 0xcafe_babf; | ||
| pub const FAT_CIGAM_64: u32 = 0xbfba_feca; |
There was a problem hiding this comment.
these are good changes, though I don't think I've ever seen this magic in the wild, though I haven't poked at macho binaries in a while
|
Thanks @m4b - I removed most changes and moved them to a new crate (goblin-ext is the working title): https://github.com/wolfv/goblin-ext I kept the things you liked. Let me know what you think! |
m4b
left a comment
m4b
left a comment
There was a problem hiding this comment.
altogether this is much more palatable; I'd like to inspect a binary with the fatarch64 struct, e.g., running bingrep on it should fail iiuc, so if you could upload one to the conversation here, i'd appreciate it
| #[derive(Clone, Copy, Default, Pread, Pwrite, SizeWith)] | ||
| /// 64-bit version of `FatArch` for fat binaries with large offsets/sizes | ||
| /// Uses u64 for offset and size fields | ||
| pub struct FatArch64 { |
There was a problem hiding this comment.
so in mod.rs we implement a FatArhcIterator, and SingleArchIterator that returns these FatArch64's; since this patch doesn't add those features, I don't see where this would be used by a client?
Again I've never seen this struct in the wild before, can you upload a binary that has it?
There was a problem hiding this comment.
@m4b thanks for the review. I added a fat64 binary to the assets and a test case. Also happy to remove all of that (could not find a single one on my system to send you, but was able to build one).
|
tool looks great! in terms of names I think goblin-patch or something like that is a little better, but if it meants to add more functionality something like ext or etc makes sense. There's also metagoblin that I maintain (which I've just remembered I have to update), but that is more like adding extra metadata to goblin that goblin itself shouldn't host/provide. 🤷 neat project of yours though! |
|
@m4b are you fine with the changes now, or should I remove the code for fat arch? I am fine either way. |
2 participants
In our application (rattler-build) we rely on
goblinto read rpaths and linked libraries, but still fall back toinstall_name_toolto actually rewrite shared libraries and binaries.I've tried to recreate the install name tool functionality identically in this PR. There is a Python script that executes the example install_name_tool (written in Rust, using Goblin) and compares the output with the proper install name tool from apple doing random modifications to binaries found on the system. The goal is to achieve bit-by-bit identical results.
I am opening this PR "early" to see if there is appetite for such functionality upstream in goblin.
Also disclaimer: most of the code was written by Claude. I am happy to spend more time to refactor things to your liking if this PR has chances to get merged!