mach/load_command: Add some missing load commands #240

woodruffw · 2020-10-11T16:58:52Z

First of all, thanks for this library! It's a pleasure to use.

I'm the maintainer of ruby-macho, and I noticed that these load commands were missing from your Mach-O implementation. I've taken the liberty of adding them; please let me know if there's anything else I can do.

Commit message:

Adds support for LC_VERSION_MIN_{TV,WATCH}OS, as well as
LC_VERSION_EXPORTS_TRIE and LC_VERSION_CHAINED_FIXUPS.
Each of the above maps to a pre-existing load command structure,
so no additional structure definitions were necessary.

Also adds constant support for LC_NOTE and LC_BUILD_VERSION.
These require new load command structures (and substructures, in the
case of LC_BUILD_VERSION), so I left them out of this PR in
the interest of brevity. I can do them in a follow-up PR,
if desired.

Adds support for `LC_VERSION_MIN_{TV,WATCH}OS`, as well as `LC_VERSION_EXPORTS_TRIE` and `LC_VERSION_CHAINED_FIXUPS`. Each of the above maps to a pre-existing load command structure, so no additional structure definitions were necessary. Also adds constant support for `LC_NOTE` and `LC_BUILD_VERSION`. These require new load command structures (and substructures, in the case of `LC_BUILD_VERSION`), so I left them out of this PR in the interest of brevity. I can do them in a follow-up PR, if desired.

willglynn

LGTM, thanks! I would welcome followup PRs that bring goblin back to parity with current #include <mach-o/*.h>.

willglynn · 2020-10-11T18:10:53Z

src/mach/load_command.rs

@@ -1014,6 +1015,7 @@ pub struct VersionMinCommand {
 }

 impl VersionMinCommand {
+    // TODO: Needs a discriminant for TvOS, WatchOS.


Yeah, we ought to break this interface. Maybe add a new #[non_exhaustive] enum Platform and add a fn platform(&self) -> Platform while we're at it.

SGTM! I can make that part of this PR.

Uses the Platform enum to supply the appropriate load command number to `VersionMinCommand`, and implements the appropriate lossless/lossy traits for round-tripping with a `u32`.

woodruffw · 2020-10-11T20:04:36Z

Okay, I've added a Platform enum and the appropriate conversion traits. One slightly messy API detail: platform() currently returns a Result<Platform> instead of a Platform directly, since u32 -> Platform is implemented with the lossy TryFrom.

It's technically safe to remove that Result with an unwrap (since the only ways to create a VersionMinCommand ensure that self.cmd is always valid), but it's difficult to prove that at the type level with the current API. Thoughts? I'm happy to go either way 🙂

woodruffw · 2020-10-11T20:06:33Z

Also, TIL that non_exhaustive was only stabilized in 1.40. That means bumping the MSRV from the current (1.36), or just dropping that attribute. Again, either's fine by me, I figure this is a question for the maintainers 🙂

Edit: I'm going to push up a commit that bumps the MSRV, just to push the changes through the CI. Happy to drop it and make the other change, if desired.

m4b · 2020-10-11T22:19:38Z

Going to review in a bit, thanks for this! So this is a breaking change right (which is fine, might as well do this now)?

woodruffw · 2020-10-11T23:01:40Z

So this is a breaking change right (which is fine, might as well do this now)?

Yep, correct -- this changes the public API for VersionMinCommand::new.

m4b

Only real question I have is whether the Platform should be repr(u32) (doesn’t have to be maybe there’s reasons to keep it away from the backing u32 consts?)

m4b · 2020-10-12T08:14:06Z

src/mach/load_command.rs

-            } else {
-                LC_VERSION_MIN_MACOSX
-            },
+            cmd: platform as u32,


Is this a new feature ? How is this cast working since Platform isn’t repr(u32).

Yep, this is part of the API breakage -- VersionMinCommand can be produced from one of four different LC_* constants, which are represented by that enum (since they might be useful elsewhere).

As for why it's working: I thought it was because I had implemented From<Platform> for u32, but I tried removing that trait impl and it still compiled 😟. So I'll change it to platform.into() to make sure it's using the trait explicitly.

m4b · 2020-10-12T08:16:42Z

src/mach/load_command.rs

+            VersionMinTvos(comm) => comm.cmdsize,
+            VersionMinWatchos(comm) => comm.cmdsize,
+            DyldExportsTrie(comm) => comm.cmdsize,
+            DyldChainedFixups(comm) => comm.cmdsize,


We don’t need to do this now but might be a good idea to add non exhaustive annotations to these enums as much well

src/mach/load_command.rs

m4b · 2020-10-12T08:19:10Z

src/mach/load_command.rs

+/// An enumeration of platforms currently identifiable within a version_min_command.
+#[non_exhaustive]
+#[derive(Debug)]
+pub enum Platform {


If these are tied to the LC values why not make it repr(u32) and set the values to the constants ? Maybe you can’t do that with constants in enums can’t remember

I thought you couldn't either, but I just tried and it works 🤷. So I'll remove the trait usage in favour of repr(u32).

m4b · 2020-10-12T08:19:57Z

README.md

@@ -20,7 +20,7 @@ https://docs.rs/goblin/

 ### Usage

-Goblin requires `rustc` 1.36.0.
+Goblin requires `rustc` 1.40.0.


I don’t really like this but it’s fine I guess.

m4b · 2020-10-12T08:22:15Z

src/mach/load_command.rs

+                Ok((DyldExportsTrie(comm), size))
+            }
+            LC_DYLD_CHAINED_FIXUPS => {
+                let comm = bytes.pread_with::<LinkeditDataCommand>(0, le)?;


This and above are both LinkEditDataCommand? Interesting

Yep, they both reside in __linkedit, like a few others.

m4b · 2020-10-12T08:32:49Z

I’m ok with bumping msrv. Re The unwrapping issue, id day if it really is safe to unwrap then do it (but use expect); but if it’s the possible, e.g. with a malformed binary, etc, to have that unwrap fail, then leave it as is :)

woodruffw · 2020-10-12T14:49:11Z

Re The unwrapping issue, id day if it really is safe to unwrap then do it (but use expect); but if it’s the possible, e.g. with a malformed binary, etc, to have that unwrap fail, then leave it as is :)

Sounds good! It should always be safe since the only ways to construct a VersionMinCommand constrain cmd to the variants we expect, so I'll use expect.

m4b

Awesome, thank you for this PR! ❤️

m4b · 2020-10-13T07:32:38Z

@woodruffw I hope you don't mind, but I squashed and merged; i find this makes PR's cleaner generally, easier to bisect, known passing commit, etc. It's not a hard and fast rule but I prefer it more and more. People do get less "commit credit" so i do give them a heads up so they don't get sad :D

woodruffw · 2020-10-13T12:32:17Z

I don’t mind at all! I usually do the same. Thanks for the quick review and merge! Sent from mobile. Please excuse my brevity.

…

On Oct 13, 2020, at 3:32 AM, m4b ***@***.***> wrote: @woodruffw I hope you don't mind, but I squashed and merged; i find this makes PR's cleaner generally, easier to bisect, known passing commit, etc. It's not a hard and fast rule but I prefer it more and more. People do get less "commit credit" so i do give them a heads up so they don't get sad :D — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

m4b · 2020-11-26T07:50:27Z

ok this is wrapped up in upcoming 0.3 release

willglynn approved these changes Oct 11, 2020

View reviewed changes

mach/load_command: Add a Platform enum

97b0c6e

Uses the Platform enum to supply the appropriate load command number to `VersionMinCommand`, and implements the appropriate lossless/lossy traits for round-tripping with a `u32`.

MSRV: 1.40.0

d41fcfe

woodruffw force-pushed the ww/more-lcs branch from 9638e21 to bb766ee Compare October 11, 2020 20:21

mach/load_command: Prefer core over std

bdfb336

woodruffw force-pushed the ww/more-lcs branch from bb766ee to bdfb336 Compare October 11, 2020 20:24

m4b approved these changes Oct 12, 2020

View reviewed changes

mach/load_command: Drop a trait, use expect

f957fe6

m4b approved these changes Oct 13, 2020

View reviewed changes

m4b merged commit b4cf6f5 into m4b:master Oct 13, 2020

woodruffw deleted the ww/more-lcs branch October 13, 2020 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mach/load_command: Add some missing load commands #240

mach/load_command: Add some missing load commands #240

woodruffw commented Oct 11, 2020

willglynn left a comment

willglynn Oct 11, 2020

woodruffw Oct 11, 2020

woodruffw commented Oct 11, 2020

woodruffw commented Oct 11, 2020 •

edited

Loading

m4b commented Oct 11, 2020

woodruffw commented Oct 11, 2020

m4b left a comment

m4b Oct 12, 2020

woodruffw Oct 12, 2020

m4b Oct 12, 2020

m4b Oct 12, 2020

woodruffw Oct 12, 2020

m4b Oct 12, 2020

m4b Oct 12, 2020

woodruffw Oct 12, 2020

m4b commented Oct 12, 2020 •

edited

Loading

woodruffw commented Oct 12, 2020

m4b left a comment

m4b commented Oct 13, 2020

woodruffw commented Oct 13, 2020 via email

m4b commented Nov 26, 2020

mach/load_command: Add some missing load commands #240

mach/load_command: Add some missing load commands #240

Conversation

woodruffw commented Oct 11, 2020

willglynn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw commented Oct 11, 2020

woodruffw commented Oct 11, 2020 • edited Loading

m4b commented Oct 11, 2020

woodruffw commented Oct 11, 2020

m4b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4b commented Oct 12, 2020 • edited Loading

woodruffw commented Oct 12, 2020

m4b left a comment

Choose a reason for hiding this comment

m4b commented Oct 13, 2020

woodruffw commented Oct 13, 2020 via email

m4b commented Nov 26, 2020

woodruffw commented Oct 11, 2020 •

edited

Loading

m4b commented Oct 12, 2020 •

edited

Loading