Duplicated LC modification logic

[woodruffw]

Currently, MachOFile#change_rpath allows a duplicate rpath to be created, since it does not check the names of rpaths already present in the file.

This behavior isn't incorrect in the sense that it's also what install_name_tool does, but it's unintuitive and will likely confuse users who expect a given rpath to occur only once. More to the point, following install_name_tools behavior will allow a user to create a binary that triggers another unintuitive behavior in install_name_tool (namely that rpath deletion only deletes the first matching rpath, not all matching rpaths).

ref #40 (comment)
cc @UniqMartin

[UniqMartin]

I think this is a good little issue to tackle once you have implemented RPATH addition and are waiting for feedback/review from me (as I sadly don't always manage to catch up with your work on a daily basis). It would certainly be nice to have all the RPATH-related features cleaned up and operating somewhat consistently.

[UniqMartin]

I guess now would be a good time to address this little issue when modifying an existing RPATH and maybe spend a few moments to think whether any of this could be relevant in the context of the #dylib_id= or #change_install_name methods.

[woodruffw]

Yep!

Unlike with rpaths, dylibs/dylib ids have associated version information that might be relevant if we decide to delete duplicated dylib commands. I'll have to do some tests on the my system libraries/homebrew-installed libs, but based on the fact that install_name_tool doesn't ask for the dylib's compatibility version we can probably assume that duplicated dylibs are treated similarly to duplicated rpaths.

[woodruffw]

I found a CMake thread from 2014 that suggests that install_name_tool used to corrupt Mach-Os containing duplicate rpaths. It seems like that behavior was quietly fixed at some point in the last two years.

This "succeeds" for me (in the sense that it doesn't corrupt the binary, but leaves one duplicate rpath behind) on 10.11.5. Do you happen to have any older OS X versions to compare with?

echo "void foo() {}" > lib.c
clang -dynamiclib -o libtest.dylib -Wl,-rpath,/usr/lib -Wl,-rpath,/usr/lib lib.c
install_name_tool -delete_rpath /usr/lib libtest.dylib
otool -L libtest.dylib

[UniqMartin]

I found a CMake thread from 2014 that suggests that install_name_tool used to corrupt Mach-Os containing duplicate rpaths.

Interesting!

Do you happen to have any older OS X versions to compare with?

I can reproduce the corruption on 10.6.8 with Xcode 3.2.6. Apparently the bug is simply that they remove both LC_RPATH commands (:+1:) and properly adjust the total size of all load commands (:+1:) but then only decrement the load command count by one (:-1:). In a way they were closer to what I would consider desirable behavior than they are today.

Here's a diff of a hex dump of the file before/after the modification:

--- libtest-original.dylib.hex  2016-07-27 19:43:33.000000000 +0100
+++ libtest.dylib.hex   2016-07-27 19:43:41.000000000 +0100
@@ -1,5 +1,5 @@
 00000000  cf fa ed fe 07 00 00 01  03 00 00 00 06 00 00 00  |................|
-00000010  0a 00 00 00 c0 02 00 00  85 00 10 00 00 00 00 00  |................|
+00000010  09 00 00 00 90 02 00 00  85 00 10 00 00 00 00 00  |................|
 00000020  19 00 00 00 38 01 00 00  5f 5f 54 45 58 54 00 00  |....8...__TEXT..|
 00000030  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
 00000040  00 10 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
@@ -41,9 +41,9 @@
 00000280  18 00 00 00 02 00 00 00  0b 02 7d 00 00 00 01 00  |..........}.....|
 00000290  2f 75 73 72 2f 6c 69 62  2f 6c 69 62 53 79 73 74  |/usr/lib/libSyst|
 000002a0  65 6d 2e 42 2e 64 79 6c  69 62 00 00 00 00 00 00  |em.B.dylib......|
-000002b0  1c 00 00 80 18 00 00 00  0c 00 00 00 2f 75 73 72  |............/usr|
-000002c0  2f 6c 69 62 00 00 00 00  1c 00 00 80 18 00 00 00  |/lib............|
-000002d0  0c 00 00 00 2f 75 73 72  2f 6c 69 62 00 00 00 00  |..../usr/lib....|
+000002b0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
+000002c0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
+000002d0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
 000002e0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
 000002f0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
 00000300  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|

[woodruffw]

In a way they were closer to what I would consider desirable behavior than they are today.

Definitely agree. This might be something worth bringing up on one of the Apple dev lists, seeing how the "fix" to the original bug will still cause unintuitive behavior for a user trying to fully remove an rpath that happens to be duplicated. Thoughts?

[UniqMartin]

Makes sense. I just have zero experience interacting with Apple directly. 😸

[woodruffw]

I sent a message off to darwin-dev, hopefully they can impart some advice 😄

In the mean time, I'm going to create a PR that corresponds to this behavior for rpath modification.

[UniqMartin]

I think with #49 merged we can close this now.