catch additional panics #574
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CBOR decoding is complex. While we catch panics at the "invoke" level anyways, catching them lower may help us avoid unintended side-effects (e.g., lock poisoning).
These tend to do lots of complex math/parsing. In most cases, a fatal error would be "safe", but: - It's definitely not safe for window post. A panic in window post verification would prevent disputes. - Catching panics and turning them into "illegal argument" errors is relatively safe as these methods are "pure".
Stebalien
commented
May 17, 2022
| }; | ||
| use filecoin_proofs_api::update::verify_empty_sector_update_proof; | ||
| use filecoin_proofs_api::{self as proofs, post, seal, ProverId, PublicReplicaInfo, SectorId}; | ||
| use filecoin_proofs_api::{self as proofs, ProverId, PublicReplicaInfo, SectorId}; |
There was a problem hiding this comment.
I started qualifying these calls because distinguishing between "our functions" and "not our functions" was getting difficult.
Stebalien
commented
May 17, 2022
| @@ -990,3 +869,168 @@ fn verify_seal(vi: &SealVerifyInfo) -> Result<bool> { | |||
| // Worst case, _some_ node falls out of sync. Better than the network halting. | |||
| .context("failed to verify seal proof") | |||
| } | |||
|
|
|||
| fn verify_post(verify_info: &WindowPoStVerifyInfo) -> Result<bool> { | |||
| .or_illegal_argument() | ||
| } | ||
|
|
||
| fn verify_aggregate_seals(aggregate: &AggregateSealVerifyProofAndInfos) -> Result<bool> { |
| .or_illegal_argument() | ||
| } | ||
|
|
||
| fn verify_replica_update(replica: &ReplicaUpdateInfo) -> Result<bool> { |
| .or_illegal_argument() | ||
| } | ||
|
|
||
| fn compute_unsealed_sector_cid( |
raulk
approved these changes
May 18, 2022
| Ok(v) => v, | ||
| Err(e) => { | ||
| log::error!("panic when decoding cbor from actor: {:?}", e); | ||
| Err(syscall_error!(IllegalArgument; "panic when decoding cbor from actor").into()) |
There was a problem hiding this comment.
I would suggest ErrorNumber::Serialization here, but read_cbor is only used for parsing syscall parameters, so ErrorNumber::IllegalArgument is probably a safe assumption to make.
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The only strictly necessary change is the window post one, as a panic there could make a proof indisputable. But there's no real issue with being careful and turning all of these panics into "illegal argument" errors as they're all well isolated.