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

Support for lossy MachO parsing #386

Merged
merged 1 commit into from
Apr 1, 2024
Merged

Support for lossy MachO parsing #386

merged 1 commit into from
Apr 1, 2024

Conversation

h33p
Copy link
Contributor

@h33p h33p commented Jan 5, 2024

Made it possible to parse MachO objects in less strict manner. However, default behavior is not changed. Therefore, there now is a new function parse_2 which accepts additional argument for lossyness.

Open questions:

  • Is the default behavior intended? If not, parse_2 could be made private.
  • Is parse_2 sufficient name, or should this be called something like parse_lossy?
  • I made all of the commands work without fail in lossy mode, however, maybe some should fail?
  • Should from_32_2 from_64_2 on Segment be exposed to the public? I currently have it pub(crate).

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.

interesting PR, thank you! so as per the comments, I don't want to expose a parse_2 name publicly, needs to be parse and parse_lossy or something, with the parse_impl getting forwarded whether it's lossy or not.

the only other consideration is whether you want to think about future lossy parsing, where a config is passed in to the lossy form, and this determines which things get dropped/ignored, etc., though that might be overkill, i don't know.

I'm also not totally sold on the lossy name, but it's ok.

Some other random alternatives that are probably worse names:

  1. strict/unstrict
  2. infallible

all said and done lossy is probably the best name?

Elf also has a similar, but slightly orthogonal support for "lazy" parsing, which doesn't parse the whole thing, but iirc, this was ostensibly to allow infallible parsing when some parts are broken/let user decide if they want to fail or not, may want to look at that, so we have a more unified api/approach to lossy/infallible/lazy parsing?

src/mach/mod.rs Outdated Show resolved Hide resolved
src/mach/segment.rs Outdated Show resolved Hide resolved
src/mach/mod.rs Outdated Show resolved Hide resolved
@h33p h33p requested a review from m4b February 3, 2024 22:21
@h33p
Copy link
Contributor Author

h33p commented Feb 3, 2024

Thanks for review, finally got around addressing your comments. Please let me know if you need anything else.

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.

Test would be nice for this if you have time, otherwise lgtm

@m4b
Copy link
Owner

m4b commented Mar 10, 2024

@h33p CI is failing on rustfmt

@m4b
Copy link
Owner

m4b commented Mar 10, 2024

once format fixed this is good to go

@h33p
Copy link
Contributor Author

h33p commented Mar 27, 2024

This should be good to go now @m4b

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!

@m4b m4b merged commit 661180f into m4b:master Apr 1, 2024
6 checks passed
@m4b
Copy link
Owner

m4b commented Apr 1, 2024

note: non-breaking

@m4b
Copy link
Owner

m4b commented Apr 27, 2024

released in 0.8.1

ideeockus pushed a commit to ideeockus/goblin that referenced this pull request Jul 29, 2024
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.

2 participants