Skip to content

Do not let tokio swallow forwarded panics #777

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

s-arash
Copy link
Contributor

@s-arash s-arash commented Jun 17, 2025

We forward enclave panics (i.e., panic if the enclave panics) inside a tokio task, which results in tokio eating up the panic. This is the exact opposite of the intended behavior of forward_panics. This PR makes the runner panic outside tokio tasks, so the panic won't be caught by tokio.

Closes #763

Taowyoo
Taowyoo previously approved these changes Jun 17, 2025
Goirad
Goirad previously approved these changes Jun 17, 2025
Copy link
Member

@Goirad Goirad left a comment

Choose a reason for hiding this comment

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

Nice!

@jethrogb
Copy link
Member

Does this work? I'd think all aborts get converted to Secondary in syscall_loop.

@s-arash
Copy link
Contributor Author

s-arash commented Jun 20, 2025

@jethrogb

Does this work? I'd think all aborts get converted to Secondary in syscall_loop.

Not all of them get converted to Secondary. If the mode is ExecutbaleMain (or Library), the abort remains an Exit:

(e, ReturnSource::Library) |

But you are correct that some aborts do get converted to Secondary (when mode is ExecutableNonMain or AsyncUsercall). To fix that, I can edit this line:

(e @ Err(EnclaveAbort::Exit { panic: None }), _)

to

(e @ Err(EnclaveAbort::Exit { panic: _ }), _) 

so that it also keeps panic-exits as is. I'm not sure why we made this distinction here in the first place.

@s-arash s-arash requested a review from jethrogb June 20, 2025 16:51
@s-arash s-arash dismissed stale reviews from Goirad and Taowyoo via bc31a51 June 23, 2025 22:16
@s-arash s-arash force-pushed the arash/forward-panic branch from bc31a51 to 67153e8 Compare June 24, 2025 17:57
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.

[BUG]: enclave-runner does not join on tokio tasks it spawn
4 participants