WIP implement macos compact unwinding info #271
Conversation
There was a problem hiding this comment.
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"))) |
There was a problem hiding this comment.
Maybe this should just return option instead of malformed? Or are unwind_info sections required to be present in a macho binary?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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..?) |
There was a problem hiding this comment.
I'd prefer to take on no more dependencies
There was a problem hiding this comment.
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, | |||
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
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.