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

Add EhFrameHdr::lookup_and_parse function #316

Merged
merged 7 commits into from
Aug 1, 2018

Conversation

roblabla
Copy link
Contributor

@roblabla roblabla commented Jul 31, 2018

Currently, EhFrameHdr::lookup returns a Pointer, which is an absolute pointer which may (or may not) point to the correct FDE. This API is used by unwind-rs to speed up stack unwinding. Unfortunately, unwind-rs requires patches to gimli (see gimli-rs/unwind-rs#6) in order to make full use of this API. This is because there is no method to parse the returned Pointer into a FrameDescriptionEntry. Unwind makes parse_cfi_entry, and uses it as such:

let fdeptr = eh_frame_hdr.table().unwrap().lookup(address, bases).unwrap();
let fdeptr = match fdeptr {
    Pointer::Direct(x) => x,
    _ => unreachable!(),
};

let entry = gimli::parse_cfi_entry(bases, *sel, &mut EndianBuf::new(unsafe { std::slice::from_raw_parts(fdeptr as *const u8, 0x1000000) }, NativeEndian)).unwrap().unwrap();
let target_fde = match entry {
    CieOrFde::Fde(fde) => Some(fde.parse(|offset| sel.cie_from_offset(bases, offset))?),
    CieOrFde::Cie(_) => unimplemented!(), // return error here probably
};
if let Some(fde) = target_fde {
    if !fde.contains(address) {
        return Err(gimli::Error::NoUnwindInfoForAddress);
    }
}

As can be seen here, there is a big fat unsafe in the middle, which sucks. I believe there is a better solution.

The complicated part is that the fdeptr is absolute, and has no length associated to it. It is, however, guaranteed to always point to an eh_frame section. Furthermore, the eh_frame_hdr has a pointer to the start of this section. As such, I propose this solution, which allows us to get rid of the unsafety.

I additionally took the liberty of adding the guarantee that the returned FramePointer will be valid (there is no need to check contains()).

I am in the process of testing this, and the implementation and doc might need a bit of refinement.

CC @main--

@coveralls
Copy link

coveralls commented Jul 31, 2018

Coverage Status

Coverage increased (+0.05%) to 84.94% when pulling 7bdfa0b on roblabla:lookup_and_parse into ed96598 on gimli-rs:master.

Copy link
Contributor

@main-- main-- left a comment

Choose a reason for hiding this comment

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

I like your solution. If you revise the error handling I think this can be merged.

Although since this is based on code I wrote I certainly wouldn't mind another opinion from someone else.

src/cfi.rs Outdated

let entry = parse_cfi_entry(bases, frame.clone(), &mut input)?;
let target_fde = match entry {
Some(CieOrFde::Fde(fde)) => Some(fde.parse(|offset| frame.cie_from_offset(bases, offset))?),
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be inefficient, one may want to cache CIEs.

Copy link
Contributor

Choose a reason for hiding this comment

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

(which, by the way, is the same reason why fde.parse() takes a closure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Should we return PartialFrameDescriptionEntry ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either this or also take a closure parameter (which has the advantage that we can keep the contains check here which is a potential footgun otherwise).

src/cfi.rs Outdated
let entry = parse_cfi_entry(bases, frame.clone(), &mut input)?;
let target_fde = match entry {
Some(CieOrFde::Fde(fde)) => Some(fde.parse(|offset| frame.cie_from_offset(bases, offset))?),
Some(CieOrFde::Cie(_)) => unimplemented!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I never was entirely sure about this (hence the unimplemented!() but I believe this should actually be unreachable.

Either way, none of these cases should panic - returning an error is the correct reaction for the gimli API.

Copy link
Contributor Author

@roblabla roblabla Jul 31, 2018

Choose a reason for hiding this comment

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

It should be unreachable!() yeah, eh_frame_hdr points to FDEs only, not CIEs (that would make no sense).

I'll check which error makes more sense to return.

src/cfi.rs Outdated
None => None
};
match target_fde {
Some(ref fde) if fde.contains(address) => Ok((*fde).clone()),
Copy link
Contributor

Choose a reason for hiding this comment

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

The ref here is required because of the pattern guard, right? You can get around the awkwardness of cloning for no reason by moving this into the match above (since the other arms of that match are dead anyways).

src/cfi.rs Outdated

let eh_frame_ptr = match self.hdr.eh_frame_ptr() {
Pointer::Direct(x) => x,
_ => unreachable!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it makes sense to deal with indirect pointers here. I certainly have never encountered any. Returning an error is probably fine.

@roblabla
Copy link
Contributor Author

roblabla commented Jul 31, 2018

I'll add an example in the doc, and then I think this is ready to merge.

@roblabla
Copy link
Contributor Author

roblabla commented Aug 1, 2018

I added a test and an example (and hopefully fixed compilation, woops). If it compiles and the tests pass, this should be ready to go.

Copy link
Collaborator

@philipc philipc left a comment

Choose a reason for hiding this comment

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

I like this solution too.

src/cfi.rs Outdated
let mut input = &mut frame.section().clone();
input.skip(offset)?;

let entry = parse_cfi_entry(bases, frame.clone(), &mut input)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unneeded clone.

src/cfi.rs Outdated
Some(CieOrFde::Fde(fde)) => Ok(fde.parse(cb)?),
Some(CieOrFde::Cie(_)) => Err(Error::NotFdePointer),
None => Err(Error::NoUnwindInfoForAddress)
}?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style nit: I think I prefer returns in the match arms instead.

src/cfi.rs Outdated
/// # let bases = BaseAddresses::default()
/// # .set_cfi(address_of_cfi_section_in_memory)
/// # .set_text(address_of_text_section_in_memory)
/// # .set_data(address_of_data_section_in_memory);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Simpler to let bases = unimplemented!();?

@roblabla
Copy link
Contributor Author

roblabla commented Aug 1, 2018

All ready.

@roblabla roblabla changed the title [WIP] Add EhFrameHdr::lookup_and_parse function Add EhFrameHdr::lookup_and_parse function Aug 1, 2018
@philipc philipc merged commit 1e063cb into gimli-rs:master Aug 1, 2018
where
F: FnMut(EhFrameOffset<R::Offset>) -> Result<CommonInformationEntry<EhFrame<R>, R, R::Offset>>
{
let fdeptr = self.lookup(address, bases)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible for this to be encoded as DW_EH_PE_pcrel? If so, we'll get this wrong, since bases.cfi will be for the eh_frame section.

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.

4 participants