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

Responsive panic about invalid user input during contract execution #329

Open
cthulhu-rider opened this issue Apr 10, 2023 · 2 comments
Open
Labels
bug Something isn't working documentation Improvements or additions to documentation I3 Minimal impact S3 Minimally significant U4 Nothing urgent

Comments

@cthulhu-rider
Copy link
Contributor

cthulhu-rider commented Apr 10, 2023

Context

Calling https://pkg.go.dev/github.com/nspcc-dev/neofs-contract/container#SetEACL threw following exception:

contract execution finished with state FAULT; exception: at instruction 3702 (SUBSTR): invalid offset

This behavior occurs similar to index out of range panic in the general Go view. While in general panic has the right to exist (developer's mistakes), code MUST NOT panic about user input.

Solution

Explore all methods which are sensitive to user input and

@cthulhu-rider cthulhu-rider added bug Something isn't working documentation Improvements or additions to documentation triage labels Apr 10, 2023
@roman-khimov
Copy link
Member

code MUST NOT panic about user input

For a contract it's OK to panic.

check correctness of the user input in code (this prevents all undesired panics) and return responsive errors

You're assuming that contracts can return (any, error), but they can't. The VM only supports one return value from public methods and either it becomes a struct{} with two fields, but this complicates the interfaces substantially and can't be handled in a generic way, or contract just panics which is an exception that in most cases leads to FAULT.

Documenting assumptions on inputs is OK, it's useful anyway.

@cthulhu-rider
Copy link
Contributor Author

cthulhu-rider commented Apr 10, 2023

@roman-khimov by

return responsive errors

I meant responsive exceptions (in Go code - panic arg). IMO it's clearer to receive missing X field fault exception than invalid offset.

cthulhu-rider added a commit to cthulhu-rider/neofs-contract that referenced this issue Apr 10, 2023
Previously `setEACL` method could throw `invalid offset` fault exception
about invalid eACL argument. This message is too generic, may occur in
different places of execution code and doesn't help caller to realize
the reason. To improve user experience, it's worth to check method
arguments before slice instructions.

Pre-check `eACL` slice length in `SetEACL` method implementation and
throw responsive panic message.

Ref nspcc-dev#329.

Signed-off-by: Leonard Lyubich <[email protected]>
cthulhu-rider added a commit to cthulhu-rider/neofs-contract that referenced this issue Apr 10, 2023
`SetEACL` method imposes requirements on the eACL parameter of a generic
byte array type. These requirements should be described in the
documentation as user manual.

Ref nspcc-dev#329.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider cthulhu-rider changed the title Prevent panic due to user input during contract execution Responsive panic about invalid user input during contract execution Apr 10, 2023
@roman-khimov roman-khimov added U4 Nothing urgent S3 Minimally significant I3 Minimal impact and removed triage labels Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation I3 Minimal impact S3 Minimally significant U4 Nothing urgent
Projects
None yet
Development

No branches or pull requests

2 participants