nspcc-dev / neofs-contract

NeoFS smart-contract
GNU General Public License v3.0
10 stars 17 forks source link

Responsive panic about invalid user input during contract execution #329

Open cthulhu-rider opened 1 year ago

cthulhu-rider commented 1 year ago

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

roman-khimov commented 1 year ago

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 commented 1 year ago

@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.