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: use T aligned pointer in TempFdArray #241

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

Conversation

Erigara
Copy link

@Erigara Erigara commented Jan 31, 2024

Fix for problem highlighted by #232.

I've observed app crash on my MacOS m1 machine with frame-pointer feature enabled due to unaligned pointer in slice_from_raw_parts.

Direct call to alloc is used to create properly aligned pointer.

ManuallyDrop<T> was used instead of T when creating TempFdArrayIterator to prevent use after drop since try_iter might be called more than once (not sure that strictly required since afaik UnresolvedFrames shouldn't allocate any memory).

@YangKeao YangKeao self-requested a review January 31, 2024 12:51
@Erigara
Copy link
Author

Erigara commented Jan 31, 2024

Formatting and clippy should be fine now :)

let len = BUFFER_LENGTH * self.flush_n;
// Expect T to be non-ZST
let layout = std::alloc::Layout::array::<ManuallyDrop<T>>(len).unwrap();
let ptr = std::alloc::alloc(layout);
Copy link
Author

Choose a reason for hiding this comment

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

If preferable Vec<MaybeUninit<ManuallyDrop<T>>> could be used here to initialize file_buffer instead of direct alloc call.

@Erigara
Copy link
Author

Erigara commented Mar 11, 2024

@YangKeao hello, do you have any estimates on reviewing this PR?

@Venryx
Copy link

Venryx commented May 18, 2024

We also experience issue #232 on rust nightly-2024-03-05, with pprof as a subdependency of pyroscope_pprofrs. (we get the error in #232 at program startup)

I've tried applying the modifications in this PR, and it appears to fully resolve the startup error in our project. (letting us now use pyroscope_pprofrs)

Is there a blocker on merging of this PR? Or other ways we can help get it validated/merged?

@shinmao
Copy link

shinmao commented Aug 18, 2024

hi @Erigara , I am wondering whether there is specific condition that could trigger this panic on unaligned pointer with from_raw_parts.

AFAIK, in any usages of ReportBuilder::build(), self.file_vec.as_ptr() as *const T will make a raw pointer to u8 cast to UnresolvedFrames, which should create an unaligned pointer and trigger panic on any Rust program (preconditon check implemented in Rust core library).

However, not all usage of ReportBuilder::build() will trigger the panic actually (e.g., call_genarate() should trigger panic?)

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