Skip to content

Remove Allocations from Panic Handler #818

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 29 commits into from
Sep 10, 2025
Merged

Remove Allocations from Panic Handler #818

merged 29 commits into from
Sep 10, 2025
+97 −7

Conversation

adamperlin
Copy link
Contributor

@adamperlin adamperlin commented Aug 22, 2025 •
edited
Loading

This PR fixes #735. The approach used here is to write the formatted panic message in chunks to the host via outb to avoid buffering on the host.

@adamperlin adamperlin force-pushed the adamperlin/panic-remove-allocations branch from c83158f to 93e8563 Compare August 22, 2025 21:18
@jprendes jprendes added the kind/bugfix For PRs that fix bugs label Aug 22, 2025
danbugs
danbugs reviewed Aug 22, 2025
View reviewed changes
@danbugs danbugs requested a review from Copilot August 22, 2025 21:39
Copilot

This comment was marked as outdated.

Sign in to view

ludfjig
ludfjig reviewed Aug 22, 2025
View reviewed changes
dblnz
dblnz reviewed Aug 25, 2025
View reviewed changes
@adamperlin adamperlin force-pushed the adamperlin/panic-remove-allocations branch 3 times, most recently from 1b0113c to 576df92 Compare August 26, 2025 05:45
@adamperlin adamperlin mentioned this pull request Aug 27, 2025
Open
@adamperlin adamperlin force-pushed the adamperlin/panic-remove-allocations branch from 2f25176 to d786619 Compare August 27, 2025 20:23
@adamperlin
Copy link
Contributor Author

adamperlin commented Aug 28, 2025 •
edited
Loading

Just a note here, this is currently blocked on fuzzing failures due to the memory leak described in #826; currently attempting to use snapshotting in the host call fuzzer to restore back to a clean memory state on each fuzzing iteration, which I will open a separate PR for.

adamperlin and others added 9 commits August 28, 2025 15:07
@adamperlin
Update cargo hash in flake.nix
3ebb69a 
Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin
Remove allocations in panic handler by formatting panic message
7ab4735 
using a new FixedStringBuf type backed by a static mut array.
Adds a test to verify that StackOverflow no longer occurs on OOM panic

Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin
Clear buffer before recursive panics
65d1676 
Add some docstrings

Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin
Update cargoHash in flake.nix
6a3b887 
Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin
Remove unused imports
b7d9e40 
Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin
Add copyright header to fixed_buf.rrs
33450d3 
Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin
Call abort_with_code_and_message directly in case of format failure i…
f96c81e 
…n panic handler to avoid any possible recursive panic

Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin @Copilot
Update src/hyperlight_host/tests/integration_test.rs
6754a7e 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Adam Perlin <adamp@nanosoft.com>
@adamperlin @Copilot
Update src/tests/rust_guests/simpleguest/src/main.rs
4b13017 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Adam Perlin <adamp@nanosoft.com>
jsturtevant
jsturtevant reviewed Sep 2, 2025
View reviewed changes
@adamperlin adamperlin force-pushed the adamperlin/panic-remove-allocations branch 2 times, most recently from 60a13c2 to 43e5480 Compare September 2, 2025 19:28
@adamperlin
Merge remote-tracking branch 'hl-origin/main' into adamperlin/panic-r…
f75d9fc 
…emove-allocations

Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin
Increase panic buffer size and add description to format error message
cf5efdb 
Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin adamperlin force-pushed the adamperlin/panic-remove-allocations branch from 9b59310 to cf5efdb Compare September 2, 2025 19:45
ludfjig
ludfjig previously approved these changes Sep 2, 2025
View reviewed changes
Copy link
Contributor

@ludfjig ludfjig left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's see what others think!

dblnz
dblnz previously approved these changes Sep 3, 2025
View reviewed changes
Copy link
Contributor

@dblnz dblnz left a comment

Choose a reason for hiding this comment

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

LGTM!

syntactically
syntactically reviewed Sep 3, 2025
View reviewed changes
@adamperlin
Create an fmt::Write implementation that writes formatted string to p…
e9f5ace 
…ort-based io directly to stream

error messages to the host and avoid needing a fixed-size buffer in the
panic handler.

Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin adamperlin dismissed stale reviews from dblnz and ludfjig via e9f5ace September 3, 2025 23:45
@adamperlin
Remove heapless dependency
12f46ba 
Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin
Merge remote-tracking branch 'hl-origin/main' into adamperlin/panic-r…
1732dce 
…emove-allocations

Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin
fmt-apply
92b1ca7 
Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin
Remove unnecessary unsafe in write_abort
b894870 
Signed-off-by: adamperlin <adamp@nanosoft.com>
@andreiltd
Copy link
Member

andreiltd commented Sep 4, 2025 •
edited
Loading

Sorry, I'm a bit late to the party but I think a similar problem exists in hyperlight_guest_bin exception handler, I'm not sure if this is something we want to address as part of this PR or follow up on this. Notably:

https://github.com/hyperlight-dev/hyperlight/blob/main/src/hyperlight_guest_bin/src/exceptions/handler.rs#L82

The additional problem in hyperlight_guest_bin is that the code is unsound because the message string is NOT null terminated but it is treated as such later in the code:

https://github.com/hyperlight-dev/hyperlight/blob/main/src/hyperlight_guest_bin/src/exceptions/handler.rs#L112

In result this I belive is UB:
https://github.com/hyperlight-dev/hyperlight/blob/main/src/hyperlight_guest/src/exit.rs#L57

@syntactically
Copy link
Member

Sorry, I'm a bit late to the party but I think a similar problem exists in hyperlight_guest_bin exception handler, I'm not sure if this is something we want to address as part of this PR or follow up on this. Notably:

https://github.com/hyperlight-dev/hyperlight/blob/main/src/hyperlight_guest_bin/src/exceptions/handler.rs#L82

I'm aware of that one and am rewriting the whole exn handler at the moment anyway, so I was just going to roll it up in a slightly larger PR with the rest of the architecture stuff. Unless you are running into an urgent issue caused by this?

@andreiltd
Copy link
Member

Hey @syntactically, nothing urgent, no :) Great to hear you're working on this.

@adamperlin
Copy link
Contributor Author

I'm glad this is being worked on too! Thank you for bringing this up @andreiltd!

@ludfjig @dblnz Could I get one more review on this? I've changed the approach based on @syntactically's suggestion so now there is no fixed-size buffer on the guest side!

@adamperlin
Merge remote-tracking branch 'hl-origin/main' into adamperlin/panic-r…
16697ed 
…emove-allocations

Signed-off-by: adamperlin <adamp@nanosoft.com>
@adamperlin adamperlin added this to the v0.10.0 milestone Sep 5, 2025
ludfjig
ludfjig previously approved these changes Sep 8, 2025
View reviewed changes
Copy link
Contributor

@ludfjig ludfjig left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I am not too familiar with how OUT currently works in the host. Maybe @danbugs could review to make sure this looks correct?

danbugs
danbugs previously approved these changes Sep 9, 2025
View reviewed changes
Copy link
Contributor

@danbugs danbugs left a comment

Choose a reason for hiding this comment

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

LGTM!

jsturtevant and others added 2 commits September 9, 2025 16:56
@jsturtevant
Merge branch 'main' into adamperlin/panic-remove-allocations
4230484
@jsturtevant @danbugs
Update src/hyperlight_guest/src/exit.rs
4844ee3 
Co-authored-by: Dan Chiarlone <danilochiarlone@gmail.com>
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
@jsturtevant jsturtevant dismissed stale reviews from ludfjig and danbugs via 4844ee3 September 9, 2025 23:57
@jsturtevant jsturtevant merged commit 6432590 into hyperlight-dev:main Sep 10, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syntactically syntactically syntactically left review comments

Copilot code review Copilot Copilot left review comments

@jsturtevant jsturtevant jsturtevant approved these changes

@ludfjig ludfjig ludfjig left review comments

@dblnz dblnz dblnz left review comments

@danbugs danbugs danbugs left review comments

@devigned devigned Awaiting requested review from devigned devigned is a code owner

@marosset marosset Awaiting requested review from marosset marosset is a code owner

@jprendes jprendes Awaiting requested review from jprendes jprendes is a code owner

@simongdavies simongdavies Awaiting requested review from simongdavies simongdavies is a code owner

Assignees
No one assigned
Labels
kind/bugfix For PRs that fix bugs
Projects
None yet
Milestone
v0.10.0
Development

Successfully merging this pull request may close these issues.

Don't alloc memory in panic handler
8 participants
@adamperlin @andreiltd @syntactically @jsturtevant @ludfjig @dblnz @danbugs @jprendes