(Correct) RPath support.

[woodruffw]

At the moment, rpath modification fails on x86_64 binaries.
I'll be looking into why that is, and this PR will serve as the location for that research (and eventual fix).

[UniqMartin]

I happen to know the reason (and why it's only affecting 64-bit architectures), but I'm not sure if you would prefer to find out yourself, just a small hint, or the full explanation. Let me know! I guess I'm not disclosing too much if I'm seeing a major refactoring of set_lc_str_in_cmd in the not too distant future …

[woodruffw]

I haven't quite figured it out yet (although I know it's related to my padding/cmdsize logic).

Don't tell me quite yet 😄

[UniqMartin]

I haven't quite figured it out yet (although I know it's related to my padding/cmdsize logic).

If you're able to say this much, you're obviously very close to understanding the root cause.

[woodruffw]

I'm surprised I hadn't caught this behavior until now - that's a sizeable bug 😅

[UniqMartin]

The “R” definitely doesn't stand for “relative”. dyld(1) talks about a “run path list” that is populated from the various LC_RPATH load commands, thus I'd make this “run path” or leave it as “runtime path”.

[woodruffw]

Corrected 👍

[UniqMartin]

You found it! 👍 It had to do with the requirement that every load command as a whole needs to have a size that is a multiple of either 4 or 8 bytes, depending on the architecture, and not just the lc_str payload it might be carrying. The previous logic worked for the LC_LOAD_DYLIB load command because its fixed part was already satisfying this constraint, but the LC_RPATH load command has a fixed part of only 12 bytes, so that's not automatically fulfilled on a 64-bit architecture.

---

I think this is fine as a temporary workaround, but looking more closely at the code after you found this issue made me realize that the set_lc_str_in_cmd code is making too many assumptions that could be easily violated and could lead to wrong results. The assumptions I'm talking about:

• That there's only one lc_str in any load command.
• That the payload of this lc_str completely fills the space between the fixed part of the load command and the end of the load command indicated by cmdsize. While this is typically the case, I think it would be perfectly legal to have a load command that consists of the fixed part, then some unnecessary zero bytes, then the lc_str payload, and finally some more zero bytes, possibly more than would be strictly necessary for the 4- or 8-byte alignment of the entire load command. (I'd like to check whether install_name_tool and otool would accept and properly handle such a file or if it is actually an error not to pack the load commands as densely as possible.)
• Finally, there's also the possible issue with non-ASCII paths (that are rare, but should be legal nonetheless) where String#bytesize != String#size. I believe such a string would produce invalid results in the current padding and length calculations, causing the slice! and insert operations to break things. (This would also require some testing and some cross checking with install_name_tool and otool.)

I believe most of these issues can be avoided if this code is radically simplified. And I think the easiest way to achieve this is to expand on your idea in #36, such that setting a path in an existing load command would effectively boil down to:

1. Clone the existing load command and change the path.
2. Serialize the modified load command to a stream of bytes.
3. Check that there's enough available space if the modified load command happens to be bigger than the one it is going to replace. Fail if that's not the case.
4. Completely remove the old load command and insert the modified one in its place. Like before, adjust the zero padding between the last load command and the actual start of the program code in the __TEXT segment as necessary.

[woodruffw]

That there's only one lc_str in any load command.
That the payload of this lc_str completely fills the space between the fixed part of the load command and the end of the load command indicated by cmdsize. While this is typically the case, I think it would be perfectly legal to have a load command that consists of the fixed part, then some unnecessary zero bytes, then the lc_str payload, and finally some more zero bytes, possibly more than would be strictly necessary for the 4- or 8-byte alignment of the entire load command. (I'd like to check whether install_name_tool and otool would accept and properly handle such a file or if it is actually an error not to pack the load commands as densely as possible.)
Finally, there's also the possible issue with non-ASCII paths (that are rare, but should be legal nonetheless) where String#bytesize != String#size. I believe such a string would produce invalid results in the current padding and length calculations, causing the slice! and insert operations to break things. (This would also require some testing and some cross checking with install_name_tool and otool.)

Yep, all of these cases are missed by set_lc_str_in_cmd.

Clone the existing load command and change the path.
Serialize the modified load command to a stream of bytes.
Check that there's enough available space if the modified load command happens to be bigger than the one it is going to replace. Fail if that's not the case.
Completely remove the old load command and insert the modified one in its place. Like before, adjust the zero padding between the last load command and the actual start of the program code in the __TEXT segment as necessary.

This approach sounds very reasonable to me, and would be a significant step towards making ruby-macho more modular and flexible 👍

In the mean time, I'll merge this (temporary) fix once you're satisfied with it.

[UniqMartin]

Feel free to merge, ideally with a FIXME or TODO comment at the top of that method that links to this discussion, though I'm hopeful you'll be able to re-engineer this fairly soon.

(And don't get me wrong, it's easy to point out flaws when you haven't written the code yourself and at least you were bold enough to write that code while all my Mach-O related code to date was read-only in fear of not understanding enough and thus breaking stuff. 😀 Absolutely appreciate your work!)

[woodruffw]

Feel free to merge, ideally with a FIXME or TODO comment at the top of that method that links to this discussion, though I'm hopeful you'll be able to re-engineer this fairly soon.

Will do!

(And don't get me wrong, it's easy to point out flaws when you haven't written the code yourself and at least you were bold enough to write that code while all my Mach-O related code to date was read-only in fear of not understanding enough and thus breaking stuff. 😀 Absolutely appreciate your work!)

Thanks, I appreciate it. Don't worry about being critical about my code, that's the only way it'll get better 😄

[UniqMartin]

Looks good to me!

Don't worry about being critical about my code, that's the only way it'll get better 😄

Awesome!