Merge pull request hyperledger-archives#961 from silasdavis/develop

Prevent SetStorage on non-existent accounts

acm/state/state_cache.go

@@ -83,8 +83,12 @@ func (cache *Cache) GetAccount(address crypto.Address) (*acm.Account, error) {
 }
 
 func (cache *Cache) UpdateAccount(account *acm.Account) error {
+	if account == nil {
+		return errors.ErrorCodef(errors.ErrorCodeIllegalWrite, "UpdateAccount called with nil account")
+	}
 	if cache.readonly {
-		return errors.ErrorCodef(errors.ErrorCodeIllegalWrite, "UpdateAccount called on read-only account %v", account.GetAddress())
+		return errors.ErrorCodef(errors.ErrorCodeIllegalWrite,
+			"UpdateAccount called in a read-only context on account %v", account.GetAddress())
 	}
 	accInfo, err := cache.get(account.GetAddress())
 	if err != nil {
@@ -93,7 +97,7 @@ func (cache *Cache) UpdateAccount(account *acm.Account) error {
 	accInfo.Lock()
 	defer accInfo.Unlock()
 	if accInfo.removed {
-		return fmt.Errorf("UpdateAccount on a removed account: %s", account.GetAddress())
+		return errors.ErrorCodef(errors.ErrorCodeIllegalWrite, "UpdateAccount on a removed account: %s", account.GetAddress())
 	}
 	accInfo.account = account.Copy()
 	accInfo.updated = true
@@ -159,16 +163,21 @@ func (cache *Cache) GetStorage(address crypto.Address, key binary.Word256) (bina
 // NOTE: Set value to zero to remove.
 func (cache *Cache) SetStorage(address crypto.Address, key binary.Word256, value binary.Word256) error {
 	if cache.readonly {
-		return errors.ErrorCodef(errors.ErrorCodeIllegalWrite, "SetStorage called on read-only account %v", address)
+		return errors.ErrorCodef(errors.ErrorCodeIllegalWrite,
+			"SetStorage called in a read-only context on account %v", address)
 	}
 	accInfo, err := cache.get(address)
+	if accInfo.account == nil {
+		return errors.ErrorCodef(errors.ErrorCodeIllegalWrite,
+			"SetStorage called on an account that does not exist: %v", address)
+	}
 	accInfo.Lock()
 	defer accInfo.Unlock()
 	if err != nil {
 		return err
 	}
 	if accInfo.removed {
-		return fmt.Errorf("SetStorage on a removed account: %s", address)
+		return errors.ErrorCodef(errors.ErrorCodeIllegalWrite, "SetStorage on a removed account: %s", address)
 	}
 	accInfo.storage[key] = value
 	accInfo.updated = true
@@ -196,7 +205,7 @@ func (cache *Cache) IterateCachedStorage(address crypto.Address,
 
 // Syncs changes to the backend in deterministic order. Sends storage updates before updating
 // the account they belong so that storage values can be taken account of in the update.
-func (cache *Cache) Sync(state Writer) error {
+func (cache *Cache) Sync(st Writer) error {
 	if cache.readonly {
 		// Sync is (should be) a no-op for read-only - any modifications should have been caught in respective methods
 		return nil
@@ -213,30 +222,31 @@ func (cache *Cache) Sync(state Writer) error {
 		accInfo := cache.accounts[address]
 		accInfo.RLock()
 		if accInfo.removed {
-			err := state.RemoveAccount(address)
+			err := st.RemoveAccount(address)
 			if err != nil {
 				return err
 			}
 		} else if accInfo.updated {
+			// First update account in case it needs to be created
+			err := st.UpdateAccount(accInfo.account)
+			if err != nil {
+				return err
+			}
+			// Sort keys
 			var keys binary.Words256
 			for key := range accInfo.storage {
 				keys = append(keys, key)
 			}
-			// First update keys
 			sort.Sort(keys)
+			// Update account's storage
 			for _, key := range keys {
 				value := accInfo.storage[key]
-				err := state.SetStorage(address, key, value)
+				err := st.SetStorage(address, key, value)
 				if err != nil {
 					return err
 				}
 			}
-			// Update account - this gives backend the opportunity to update StorageRoot after calculating the new
-			// value from any storage value updates
-			err := state.UpdateAccount(accInfo.account)
-			if err != nil {
-				return err
-			}
+
 		}
 		accInfo.RUnlock()
 	}

acm/state/state_cache_test.go

@@ -193,9 +193,9 @@ func TestStateCache_SetStorage(t *testing.T) {
 
 	//Create new account and set its storage in cache
 	newAcc := acm.NewAccountFromSecret("newAcc")
-	err := cache.SetStorage(newAcc.Address, word("What?"), word("Huh?"))
+	err := cache.UpdateAccount(newAcc)
 	require.NoError(t, err)
-	err = cache.UpdateAccount(newAcc)
+	err = cache.SetStorage(newAcc.Address, word("What?"), word("Huh?"))
 	require.NoError(t, err)
 
 	//Check for correct cache storage value
@@ -211,22 +211,33 @@ func TestStateCache_SetStorage(t *testing.T) {
 	newAccStorage, err = backend.GetStorage(newAcc.Address, word("What?"))
 	require.NoError(t, err)
 	assert.Equal(t, word("Huh?"), newAccStorage)
+
+	noone := acm.NewAccountFromSecret("noone at all")
+	err = cache.SetStorage(noone.Address, binary.Word256{3, 4, 5}, binary.Word256{102, 103, 104})
+	require.Error(t, err, "should not be able to write to non-existent account")
+
+	err = cache.UpdateAccount(noone)
+	require.NoError(t, err)
+	err = cache.SetStorage(noone.Address, binary.Word256{3, 4, 5}, binary.Word256{102, 103, 104})
+	require.NoError(t, err, "should be able to update account after creating it")
 }
 
 func TestStateCache_Sync(t *testing.T) {
 	// Build backend states for read and write
 	backend := NewCache(NewMemoryState())
 	cache := NewCache(backend)
 
-	//Create new account
+	// Create new account
 	newAcc := acm.NewAccountFromSecret("newAcc")
+	// Create account
+	err := cache.UpdateAccount(newAcc)
 
-	//Set balance for account
+	// Set balance for account
 	balance := uint64(24)
 	newAcc.Balance = balance
 
-	//Set storage for account
-	err := cache.SetStorage(newAcc.Address, word("God save"), word("the queen!"))
+	// Set storage for account
+	err = cache.SetStorage(newAcc.Address, word("God save"), word("the queen!"))
 	require.NoError(t, err)
 
 	//Update cache with account changes
@@ -295,7 +306,6 @@ func TestStateCache_get(t *testing.T) {
 	newAccOut, err = backend.GetAccount(newAcc.Address)
 	require.NoError(t, err)
 	require.NotNil(t, newAccOut)
-
 }
 
 func testAccounts() *MemoryState {

deploy/jobs/jobs_contracts.go

@@ -9,11 +9,12 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/hyperledger/burrow/execution/errors"
+
 	"github.com/hyperledger/burrow/crypto"
 	compilers "github.com/hyperledger/burrow/deploy/compile"
 	"github.com/hyperledger/burrow/deploy/def"
 	"github.com/hyperledger/burrow/deploy/util"
-	"github.com/hyperledger/burrow/execution/errors"
 	"github.com/hyperledger/burrow/execution/evm/abi"
 	"github.com/hyperledger/burrow/txs/payload"
 	log "github.com/sirupsen/logrus"
@@ -417,23 +418,29 @@ func CallJob(call *def.Call, tx *payload.CallTx, do *def.DeployArgs, client *def
 		return "", nil, err
 	}
 
-	if txe.Exception != nil && txe.Exception.ErrorCode() == errors.ErrorCodeExecutionReverted {
-		message, err := abi.UnpackRevert(txe.Result.Return)
-		if err != nil {
-			return "", nil, err
-		}
-		if message != nil {
-			log.WithField("Revert Reason", *message).Error("Transaction reverted with reason")
-			return *message, nil, txe.Exception.AsError()
-		} else {
-			log.Error("Transaction reverted with no reason")
+	if txe.Exception != nil {
+		switch txe.Exception.ErrorCode() {
+		case errors.ErrorCodeExecutionReverted:
+			message, err := abi.UnpackRevert(txe.Result.Return)
+			if err != nil {
+				return "", nil, err
+			}
+			if message != nil {
+				log.WithField("Revert Reason", *message).Error("Transaction reverted with reason")
+				return *message, nil, txe.Exception.AsError()
+			} else {
+				log.Error("Transaction reverted with no reason")
+				return "", nil, txe.Exception.AsError()
+			}
+		default:
+			log.Error("Transaction execution exception")
 			return "", nil, txe.Exception.AsError()
 		}
 	}
 	var result string
 
 	// Formally process the return
-	if txe.Result.Return != nil {
+	if txe.GetResult().GetReturn() != nil {
 		log.Debug(txe.Result.Return)
 
 		log.WithField("=>", result).Debug("Decoding Raw Result")

execution/evm/vm.go

@@ -100,14 +100,13 @@ func HasPermission(st Interface, address crypto.Address, perm permission.PermFla
 	return value
 }
 
-func EnsurePermission(st Interface, address crypto.Address, perm permission.PermFlag) errors.CodedError {
+func EnsurePermission(st Interface, address crypto.Address, perm permission.PermFlag) {
 	if !HasPermission(st, address, perm) {
-		return errors.PermissionDenied{
+		st.PushError(errors.PermissionDenied{
 			Address: address,
 			Perm:    perm,
-		}
+		})
 	}
-	return nil
 }
 
 func (vm *VM) fireCallEvent(eventSink EventSink, callType exec.CallType, errProvider errors.Provider, output *[]byte,
@@ -745,26 +744,36 @@ func (vm *VM) execute(callState Interface, eventSink EventSink, caller, callee c
 			callState.IncSequence(callee)
 			newAccount := crypto.NewContractAddress(callee, callState.GetSequence(callee))
 
+			// Check the CreateContract permission for this account
+			EnsurePermission(callState, callee, permission.CreateContract)
+			if callState.Error() != nil {
+				continue
+			}
+
+			// Establish a frame in which the putative account exists
+			childCallState := callState.NewCache()
+			create(childCallState, newAccount)
+
 			// Run the input to get the contract code.
 			// NOTE: no need to copy 'input' as per Call contract.
-			ret, callErr := vm.Call(callState, eventSink, callee, newAccount, input, input, contractValue, gas)
+			ret, callErr := vm.Call(childCallState, eventSink, callee, newAccount, input, input, contractValue, gas)
 			if callErr != nil {
 				stack.Push(Zero256)
 				// Note we both set the return buffer and return the result normally
 				returnData = ret
 			} else {
-				callState.PushError(createContract(callState, callee, newAccount, ret))
+				// Update the account with its initialised contract code
+				childCallState.InitCode(newAccount, ret)
+				callState.PushError(childCallState.Sync())
 				stack.PushAddress(newAccount)
 			}
 
 		case CALL, CALLCODE, DELEGATECALL, STATICCALL: // 0xF1, 0xF2, 0xF4, 0xFA
 			returnData = nil
 
-			permErr := EnsurePermission(callState, callee, permission.Call)
-			if permErr != nil {
-				callState.PushError(permErr)
+			EnsurePermission(callState, callee, permission.Call)
+			if callState.Error() != nil {
 				continue
-
 			}
 			gasLimit := stack.PopU64()
 			address := stack.PopAddress()
@@ -820,9 +829,8 @@ func (vm *VM) execute(callState Interface, eventSink EventSink, caller, callee c
 						continue
 					}
 					// We're sending funds to a new account so we must create it first
-					createErr := createAccount(callState, callee, address)
-					if createErr != nil {
-						callState.PushError(createErr)
+					createAccount(callState, callee, address)
+					if callState.Error() != nil {
 						continue
 					}
 				}
@@ -904,17 +912,16 @@ func (vm *VM) execute(callState Interface, eventSink EventSink, caller, callee c
 			if !callState.Exists(receiver) {
 				// If receiver address doesn't exist, try to create it
 				useGasNegative(gas, GasCreateAccount, callState)
-				createErr := createAccount(callState, callee, receiver)
-				if createErr != nil {
-					callState.PushError(createErr)
+				createAccount(callState, callee, receiver)
+				if callState.Error() != nil {
 					continue
 				}
 			}
 			balance := callState.GetBalance(callee)
 			callState.AddToBalance(receiver, balance)
 			callState.RemoveAccount(callee)
 			vm.Debugf(" => (%X) %v\n", receiver[:4], balance)
-			fallthrough
+			return nil
 
 		case STOP: // 0x00
 			return nil
@@ -933,33 +940,17 @@ func (vm *VM) execute(callState Interface, eventSink EventSink, caller, callee c
 	return
 }
 
-func createAccount(st Interface, creator, address crypto.Address) errors.CodedError {
-	err := EnsurePermission(st, creator, permission.CreateAccount)
-	if err != nil {
-		return err
-	}
-	return create(st, address, nil)
+func createAccount(st Interface, creator, address crypto.Address) {
+	EnsurePermission(st, creator, permission.CreateAccount)
+	create(st, address)
 }
 
-func createContract(st Interface, creator, address crypto.Address, code []byte) errors.CodedError {
-	err := EnsurePermission(st, creator, permission.CreateContract)
-	if err != nil {
-		return err
-	}
-	return create(st, address, code)
-}
-
-func create(st Interface, address crypto.Address, code []byte) errors.CodedError {
+func create(st Interface, address crypto.Address) {
 	if IsRegisteredNativeContract(address) {
-		return errors.ErrorCodef(errors.ErrorCodeReservedAddress,
-			"cannot create account at %v because that address is reserved for a native contract", address)
+		st.PushError(errors.ErrorCodef(errors.ErrorCodeReservedAddress,
+			"cannot create account at %v because that address is reserved for a native contract", address))
 	}
 	st.CreateAccount(address)
-	st.InitCode(address, code)
-	if st.Error() != nil {
-		return errors.Wrap(st.Error(), "createAccount could not create account")
-	}
-	return nil
 }
 
 // Returns a subslice from offset of length length and a bool

execution/execution.go

@@ -212,6 +212,14 @@ func (exe *executor) Execute(txEnv *txs.Envelope) (txe *exec.TxExecution, err er
 	if txExecutor, ok := exe.contexts[txEnv.Tx.Type()]; ok {
 		// Establish new TxExecution
 		txe := exe.blockExecution.Tx(txEnv)
+		defer func() {
+			if r := recover(); r != nil {
+				err = fmt.Errorf("recovered from panic in executor.Execute(%s): %v\n%s", txEnv.String(), r,
+					debug.Stack())
+				// If we recover here we are in a position to promulgate the error to the TxExecution
+				txe.PushError(err)
+			}
+		}()
 
 		// Validate inputs and check sequence numbers
 		err = validateInputs(txEnv.Tx, exe.stateCache)

project/history.go

@@ -47,7 +47,23 @@ func FullVersion() string {
 // To cut a new release add a release to the front of this slice then run the
 // release tagging script: ./scripts/tag_release.sh
 var History relic.ImmutableHistory = relic.NewHistory("Hyperledger Burrow", "https://github.com/hyperledger/burrow").
-	MustDeclareReleases("0.23.0 - 2018-11-09",
+	MustDeclareReleases("",
+		`### Security
+
+### Changed
+
+### Fixed
+- [EVM] state/Cache no longer allows SetStorage on accounts that do not exist
+
+### Added
+- [Execution] panics from executors are captured and pushed to error sink of TxExecution
+
+### Removed
+
+### Deprecated
+
+`,
+		"0.23.0 - 2018-11-09",
 		`### Changed
 - [ABI] provides fast event lookup of EventID
 - [Events] BlockExecution now included full Tendermint block header as protobuf object rather than JSON string