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

Fix panic in parsing UnwindInfo #218

Merged
merged 1 commit into from
May 9, 2020

Conversation

jan-auer
Copy link
Contributor

@jan-auer jan-auer commented May 5, 2020

The exception handler address incorrectly shadowed the offset for reading exception handler data. This could lead to out-of-bounds access which and results in a panic.

Instead, the address should be treated as an opaque pointer (relative, probably RVA), while the data follows the pointer. See https://docs.microsoft.com/en-us/cpp/build/exception-handling-x64

@jan-auer jan-auer changed the title Fix invalid offset for unwind exception handler data Fix panic in parsing UnwindInfo May 5, 2020
@@ -587,13 +587,13 @@ impl<'a> UnwindInfo<'a> {
// whenever flags UNW_FLAG_EHANDLER or UNW_FLAG_UHANDLER are set. The language-specific
// handler is called as part of the search for an exception handler or as part of an unwind.
} else if flags & (UNW_FLAG_EHANDLER | UNW_FLAG_UHANDLER) != 0 {
let offset = bytes.gread_with::<u32>(&mut offset, scroll::LE)? as usize;
let address = bytes.gread_with::<u32>(&mut offset, scroll::LE)?;
let data = &bytes[offset..];
Copy link
Owner

Choose a reason for hiding this comment

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

should data use offset or address here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for asking, data should in fact use offset.

For posterity: This is documented here. With the handler flags set, struct UNWIND_INFO contains a u32 address of the exception handler, followed immediately by variable length data.

@m4b
Copy link
Owner

m4b commented May 8, 2020

Once confirm offset is ok @jan-auer ill merge this and push a new release with the other changes from @jackcmay ; i think i turned off the travis-ci job too, so hopefully that won't be annoying anymore.

@m4b m4b merged commit 189ea53 into m4b:master May 9, 2020
@m4b
Copy link
Owner

m4b commented May 9, 2020

thanks for the PR @jan-auer

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