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

mach.parse: Handle DyldExportsTrie #303

Merged
merged 2 commits into from
Apr 10, 2022
Merged

mach.parse: Handle DyldExportsTrie #303

merged 2 commits into from
Apr 10, 2022

Conversation

apalm
Copy link
Contributor

@apalm apalm commented Feb 27, 2022

Fixes missing exports.

Fixes missing exports.
@@ -294,6 +294,30 @@ impl<'a> ExportTrie<'a> {
location,
}
}

/// Create a new, lazy, zero-copy export trie from the `LinkeditDataCommand` `command`
pub fn new_from_linkedit_data_command(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably, we don't want a variation of the new function, but was simple to implement. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is kind of long, but fine; however, can you dispatch both of these implemenations to a new private constructor, say new_impl which takes the bytes + the offset + size, since that is how they only differ, e.g.:

fn new_impl(bytes: &'a [u8], offset: usize, size: usize) -> Self {
 // etc.
}

pub fn new(bytes: &'a [u8], command: &load_command::DyldInfoCommand) -> Self {
  Self::new_impl(bytes, command.export_off as usize, ommand.export_size as usize)
}

// one more using the LinkeditDataComand

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m4b Updated.

/// Create a new, lazy, zero-copy export trie from the `LinkeditDataCommand` `command`
pub fn new_from_linkedit_data_command(
bytes: &'a [u8],
command: &load_command::LinkeditDataCommand,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to your PR< but i just noticed the capitalization for this is strange, surprised it's not LinkEditDataCommand; is this how it occurs in mach sources?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given it's LINKEDIT, not LINK_EDIT, I think the current capitalization makes sense?

@@ -294,6 +294,30 @@ impl<'a> ExportTrie<'a> {
location,
}
}

/// Create a new, lazy, zero-copy export trie from the `LinkeditDataCommand` `command`
pub fn new_from_linkedit_data_command(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is kind of long, but fine; however, can you dispatch both of these implemenations to a new private constructor, say new_impl which takes the bytes + the offset + size, since that is how they only differ, e.g.:

fn new_impl(bytes: &'a [u8], offset: usize, size: usize) -> Self {
 // etc.
}

pub fn new(bytes: &'a [u8], command: &load_command::DyldInfoCommand) -> Self {
  Self::new_impl(bytes, command.export_off as usize, ommand.export_size as usize)
}

// one more using the LinkeditDataComand

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

@m4b m4b merged commit 948b02a into m4b:master Apr 10, 2022
@m4b
Copy link
Owner

m4b commented Apr 10, 2022

@apalm do you need a release urgently? I like to roll up a few of these non-breaking changes before another patch release

@apalm
Copy link
Contributor Author

apalm commented Apr 10, 2022

@m4b Nope, not urgent :)

@lygstate
Copy link

lygstate commented May 9, 2022

What's src/.DS_Store for ?

@m4b
Copy link
Owner

m4b commented May 23, 2022

@lygstate good catch, sorry that i missed this :/ i've deleted it, though it's a bit sad it'll be in the git history forever; i could force push it out, but i don't think it's worth breaking anyone with an upstream clone

@m4b
Copy link
Owner

m4b commented May 23, 2022

actually, i've changed my mind, i'm going to force push it out

m4b pushed a commit that referenced this pull request May 23, 2022
* Fixes missing exports.
* Add `new_impl` to `ExportTrie`
@lygstate
Copy link

actually, i've changed my mind, i'm going to force push it out

better add it into gitignore

@m4b
Copy link
Owner

m4b commented Jun 6, 2022

ok released in 0.5.2, thanks for your help!

and thanks for your patience waiting for this!

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.

3 participants