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

WIP implement macos compact unwinding info #271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gankra
Copy link

@Gankra Gankra commented Apr 27, 2021

This is a port of getsentry/symbolic#372 to move the core of the implementation into goblin, per @jan-auer's suggestion that it should live in this crate.

I haven't really properly implemented the main entry point that's hanging off of the Macho type, as I'm not sure what the right way to grab the target architecture is.

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.

It's a good chunk of code! :D Let me know when it's no longer WIP, and ready to be considered for merging/review, and I'll get to the rest then; otherwise, lgtm generally speaking; as I mentioned in a comment, I would greatly prefer no more dependencies being added than what we already have

if let Some((_header, data)) = get_unwind_info_section() {
compact_unwind_info::CompactUnwindInfoIter::new(data, self.little_endian, self.is_64)
} else {
Err(error::Error::Malformed(String::from("No unwind_info section found")))
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe this should just return option instead of malformed? Or are unwind_info sections required to be present in a macho binary?

Copy link
Author

Choose a reason for hiding this comment

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

Yes that's a good idea. At the time it felt strange to have Result<Option<...>> but now it's totally reasonable to me.

for section in segment {
if let Ok((header, data)) = section {
if let Ok(sec) = header.name() {
if sec.len() >= 2 && &sec[2..] == section_name {
Copy link
Owner

Choose a reason for hiding this comment

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

why &sec[2..] is there some ascii prefix it is stripping here? If it's always the same, maybe it's better to have section_name contain that prefix and remove this part?

Copy link
Contributor

@jan-auer jan-auer May 3, 2021

Choose a reason for hiding this comment

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

FWIW, that's the two underscores, for example "__unwind_info". This function is structured similarly to object and symbolic which allow to search in both ELF and MachO for a section name regardless of the specific prefix.

Copy link
Author

Choose a reason for hiding this comment

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

(yea this is a straight-up copy-paste of symbolic to resolve the section by name, since that functionality seemed to be absent.)

iter: &CompactUnwindInfoIter,
) -> Option<std::vec::IntoIter<CfiOp>> {
let pointer_size = self.pointer_size(iter) as i32;
// TODO: don't allocate for this (use ArrayVec..?)
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to take on no more dependencies

Copy link
Author

Choose a reason for hiding this comment

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

Ok! In the original PR on symbolic I have now replaced the Vec with a little adhoc ArrayVec impl to get rid of the Vec without introducing that dependency.

@@ -0,0 +1,1503 @@
//! Support for the "compact unwinding format" used by Apple platforms,
Copy link
Owner

Choose a reason for hiding this comment

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

I love the documentation! So this file will take me a bit of time to review, thankfully it looks like half of it is documentation. It might also help to get another expert to review, any help appreciated :) I will probably hold off doing a review until this PR is ready to be non WIP though, so please do let me know when you think it's ready :)

Copy link
Author

Choose a reason for hiding this comment

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

I have been iterating on this back in the original PR on symbolic and I believe it is in its ~final form, modulo what everyone wants me to do with the outstanding TODOs that can potentially be deferred for followups.

Just to avoid constantly synching things over and over, I'd like all review to go on there and then when everyone's happy I'll port the impl back over to goblin and we can discuss the implementation/signature of the entry point in mach.

The large outstanding question for me for integrating what I have into goblin is how to, using only goblin's API, detect the architecture (x86, x64, ARM64) which is otherwise not specified in the unwind_info section but determines how opcodes are interpreted.

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