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

std: validate frame-pointer address in stack walking #10863

Merged
merged 2 commits into from Feb 13, 2022
Merged

std: validate frame-pointer address in stack walking #10863

merged 2 commits into from Feb 13, 2022

Conversation

mateuszradomski
Copy link
Contributor

Before we are going to de-reference the frame-pointer check if the memory is mapped. Inspired by libunwind. For now this is just a draft, main thing that is not finished is the same behavior on non-POSIX systems (Windows). I'm open to input :+]

Closes: #10114.

@mateuszradomski mateuszradomski marked this pull request as ready for review February 12, 2022 18:16
@andrewrk andrewrk merged commit 5f50980 into ziglang:master Feb 13, 2022
@andrewrk
Copy link
Member

Nice improvement!

const aligned_address = address & ~@intCast(usize, (mem.page_size - 1));

// If the address does not span 2 pages, query only the first one
const length: usize = if (aligned_address == address) mem.page_size else 2 * mem.page_size;
Copy link
Member

Choose a reason for hiding this comment

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

@m-radomski I'm trying to understand post factum, what is the argument here for checking if not only the current page but also the subsequent one are currently mapped by the OS? Put it another way, msync will fail should a single page out of the two we are syncing were not mapped. Are we guaranteed this will never happen? On arm64 macOS for instance the page size is a tremendous 16KB meaning we might not map two subsequent pages and as a result not printing the stack trace on encountering a trap since msync and by extension isValidMemory return error/false. Additionally, what is the point of syncing more than one page given that we will issue a call to msync on every iteration of the stack trace walker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, sorry for the late response, but the logic was ported from libunwind and after looking closely I think you are right. It ought to check just the single page. I think my go-to-definition got lost and transported me to a file for loongarch64 instead of x86. For anyone that might still be interested, search for "validate_mem()" function in libunwind.

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.

Stack iterator may segfault on non-standard stack frames
4 participants