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: restrict actor exit codes to non-system codes #3656

Closed
wants to merge 3 commits into from

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Sep 8, 2020

This PR restricts actor methods from exiting with a system exit code. It changes Runtime.Abortf to only accept non-system and non-negative exit codes. If an actor attempts to exit with a system exit code it is intercepted and converted to a SysErrorIllegalActor code.

Instances where Runtime.Abortf was being called with a system error code have been changed to call a new package private function vmabortf, which allows aborting with all error codes.

The test vectors added here assert that these changes do prevent actors from exiting with system exit codes.

refs filecoin-project/test-vectors#15

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

I think this is heading in a good direction! Let's loop in @Kubuxu for an early review.

chain/actors/aerrors/error.go Outdated Show resolved Hide resolved
chain/vm/runtime.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

SGWM

@alanshaw alanshaw marked this pull request as ready for review September 8, 2020 12:29
@whyrusleeping
Copy link
Member

This is a consensus breaking change I believe

@Kubuxu Kubuxu added the impact/consensus Impact: Consensus label Sep 10, 2020
@Kubuxu
Copy link
Contributor

Kubuxu commented Sep 10, 2020

@alanshaw another red label for you

@alanshaw alanshaw mentioned this pull request Sep 16, 2020
21 tasks
@alanshaw alanshaw requested a review from arajasek as a code owner May 7, 2021 01:26
@Stebalien
Copy link
Member

I don't think this can ever work. E.g.:

  1. A invokes some method on actor B.
  2. Actor B fails with a system exit code. E.g., because actor A isn't allowed to call it.
  3. Actor A may then "propagate" the abort and exit with the same code.

@Stebalien Stebalien closed this Jul 23, 2021
@Stebalien
Copy link
Member

(please reopen if I'm wrong)

@Kubuxu Kubuxu deleted the fix/restrict-actor-exit-codes branch November 25, 2021 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/consensus Impact: Consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants