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.

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

a panic should never unwind the stack past the FFI #553

Closed
laser opened this issue Mar 13, 2019 · 2 comments
Closed

a panic should never unwind the stack past the FFI #553

laser opened this issue Mar 13, 2019 · 2 comments
Assignees
@vmx
Labels
A-filecoin-proofs good first issue Good for newcomers

Comments

@laser
Copy link
Contributor

laser commented Mar 13, 2019 •
edited

Description

go-filecoin depends on rust-fil-proofs via CGO. A panic in rust-fil-proofs should not propagate to go-filecoin where it cannot be handled.

Acceptance criteria

Trap all panics at the FFI boundary. Transmute panic to an error message, and send that error message back to FFI consumer.

Where to start

This portion of the Rust docs outline an approach for catching panics before propagating back to FFI consumers. This blog post gives some examples of how we might use the UnwindSafe trait.
@nicola nicola mentioned this issue Mar 21, 2019
Closed
@laser laser self-assigned this May 6, 2019
@laser laser added the good first issue Good for newcomers label May 6, 2019
vmx added a commit that referenced this issue May 31, 2019
@vmx
WIP: Catch panics
cf41528 
This code currently doesn't even compile. But I upload it though, in case
someone else would like to push that over the finish line.

Closes #553.
@vmx vmx mentioned this issue May 31, 2019
Closed
@laser laser removed their assignment Jun 4, 2019
@vmx vmx mentioned this issue Jun 21, 2019
Closed
vmx added a commit to filecoin-project/rust-fil-sector-builder that referenced this issue Jun 23, 2019
@vmx
WIP: Catch panics
3a34eda 
This is a new WIP about catching panics. If we can agree that this is the
way to go, it would make sense to put this macro into rust-fil-ffi-toolkit.

This patch is best viewed with `git diff --ignore-all-space`.

Closes filecoin-project/rust-fil-proofs#553.
@vmx vmx mentioned this issue Jun 23, 2019
Closed
vmx added a commit to filecoin-project/rust-fil-sector-builder that referenced this issue Jun 23, 2019
@vmx
WIP: Catch panics
e948cde 
This is a new WIP about catching panics. If we can agree that this is the
way to go, it would make sense to put this macro into rust-fil-ffi-toolkit.

This patch is best viewed with `git diff --ignore-all-space`.

Closes filecoin-project/rust-fil-proofs#553.
@laser laser assigned laser and vmx and unassigned laser Jul 19, 2019
@laser
Copy link
Contributor Author

laser commented Jul 23, 2019

Also see @vmx PR to cbdingen: mozilla/cbindgen#347 (comment)

@laser laser self-assigned this Jul 23, 2019
@laser laser removed their assignment Sep 17, 2019
vmx added a commit to filecoin-project/rust-fil-ffi-toolkit that referenced this issue Oct 17, 2019
@vmx
feat(ffi-toolkit): add closure to catch panics
953d7e5 
When panics happen on the Rust side of the FFI, normally it just aborts
and the caller has no chance to catch the error.

When a a FFI function is wrapped in a `catch_panic_response()` closure,
then whenever a panic (where unwinding is possible) occurs it will respond
to the caller with the `FCPResponseStatus::FCPUnclassifiedError` error code
and the error message prefixed with "Rust panic: ". Now the caller can
react accordingly.

This is part of filecoin-project/rust-fil-proofs#553
@vmx vmx mentioned this issue Oct 17, 2019
Merged
vmx added a commit to filecoin-project/rust-fil-proofs-ffi that referenced this issue Oct 17, 2019
@vmx
fix: catch panics
5369b9f 
When panics happen, don't just abort, but catch them and send an error
response back with status code `FCPResponseStatus::FCPUnclassifiedError`
and an error message prefixed with "Rust panic: ".

This is part of filecoin-project/rust-fil-proofs#553
vmx added a commit to filecoin-project/rust-fil-proofs-ffi that referenced this issue Oct 23, 2019
@vmx
fix: catch panics
cb14b6c 
When panics happen, don't just abort, but catch them and send an error
response back with status code `FCPResponseStatus::FCPUnclassifiedError`
and an error message prefixed with "Rust panic: ".

This is part of filecoin-project/rust-fil-proofs#553
laser pushed a commit to filecoin-project/rust-fil-proofs-ffi that referenced this issue Oct 23, 2019
@vmx @laser
Improve error handling (#29)
f99a811 
* feat: use enum for FFI response status

Use the central `FCPResponseStatus` enum from ffi-toolkit for the
FFI responses.

Closes #8.

* fix: catch panics

When panics happen, don't just abort, but catch them and send an error
response back with status code `FCPResponseStatus::FCPUnclassifiedError`
and an error message prefixed with "Rust panic: ".

This is part of filecoin-project/rust-fil-proofs#553
vmx added a commit to filecoin-project/rust-fil-sector-builder that referenced this issue Oct 23, 2019
@vmx
fix: catch panics
c02cceb 
When panics happen, don't just abort, but catch them and send an error
response back with status code `FCPResponseStatus::FCPUnclassifiedError`
and an error message prefixed with "Rust panic: ".

This is part of filecoin-project/rust-fil-proofs#553

TODO: update Cargo.lock
vmx added a commit to filecoin-project/rust-fil-sector-builder that referenced this issue Oct 23, 2019
@vmx
fix: catch panics
e56313f 
When panics happen, don't just abort, but catch them and send an error
response back with status code `FCPResponseStatus::FCPUnclassifiedError`
and an error message prefixed with "Rust panic: ".

This is part of filecoin-project/rust-fil-proofs#553
vmx added a commit to filecoin-project/rust-fil-sector-builder that referenced this issue Oct 28, 2019
@vmx
fix: catch panics
6397a93 
When panics happen, don't just abort, but catch them and send an error
response back with status code `FCPResponseStatus::FCPUnclassifiedError`
and an error message prefixed with "Rust panic: ".

This is part of filecoin-project/rust-fil-proofs#553
vmx added a commit to filecoin-project/rust-fil-sector-builder that referenced this issue Oct 28, 2019
@vmx
fix: catch panics
c6c9c85 
When panics happen, don't just abort, but catch them and send an error
response back with status code `FCPResponseStatus::FCPUnclassifiedError`
and an error message prefixed with "Rust panic: ".

This is part of filecoin-project/rust-fil-proofs#553
vmx added a commit to filecoin-project/rust-fil-sector-builder that referenced this issue Oct 30, 2019
@vmx
fix: catch panics
3b934b9 
When panics happen, don't just abort, but catch them and send an error
response back with status code `FCPResponseStatus::FCPUnclassifiedError`
and an error message prefixed with "Rust panic: ".

This is part of filecoin-project/rust-fil-proofs#553
laser pushed a commit to filecoin-project/rust-fil-sector-builder that referenced this issue Oct 30, 2019
@vmx @laser
fix: catch panics (#72)
c62c649 
When panics happen, don't just abort, but catch them and send an error
response back with status code `FCPResponseStatus::FCPUnclassifiedError`
and an error message prefixed with "Rust panic: ".

This is part of filecoin-project/rust-fil-proofs#553
@vmx
Copy link
Contributor

vmx commented Nov 12, 2019

I close this issue as fil-proofs-ffi and sector-builder-ffi now have their panics caught.

@vmx vmx closed this as completed Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vmx vmx

Labels
A-filecoin-proofs good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
3 participants
@vmx @dignifiedquire @laser