From 40e24de29e56c7d06af7c6bf98c4525c89877c68 Mon Sep 17 00:00:00 2001 From: "Mr. Is" Date: Tue, 9 May 2017 11:12:48 -0500 Subject: [PATCH 1/5] problem: sigsegv crash on malformed chainidsigner solution: ensure non-zero value for chainids before comparing them. tests passing. fixes #218 --- core/types/transaction_signing.go | 12 +++++++++++- core/types/transaction_signing_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/core/types/transaction_signing.go b/core/types/transaction_signing.go index 2b79ad5e9..d5609ab15 100644 --- a/core/types/transaction_signing.go +++ b/core/types/transaction_signing.go @@ -134,7 +134,17 @@ func NewChainIdSigner(chainId *big.Int) ChainIdSigner { func (s ChainIdSigner) Equal(s2 Signer) bool { other, ok := s2.(ChainIdSigner) - return ok && other.chainId.Cmp(s.chainId) == 0 + if !ok { + return false + } + if other.chainId.Cmp(new(big.Int)) == 0 || other.chainId == nil { + return false + } + if s.chainId.Cmp(new(big.Int)) == 0 || s.chainId == nil { + return false + } + + return other.chainId.Cmp(s.chainId) == 0 } func (s ChainIdSigner) SignECDSA(tx *Transaction, prv *ecdsa.PrivateKey) (*Transaction, error) { diff --git a/core/types/transaction_signing_test.go b/core/types/transaction_signing_test.go index eaf1d345d..4a6f92f08 100644 --- a/core/types/transaction_signing_test.go +++ b/core/types/transaction_signing_test.go @@ -135,3 +135,28 @@ func TestCompatibleSign(t *testing.T) { t.Errorf("Incorrect pubkey for Basic Signer:\n%v\n%v", common.ToHex(pub), common.ToHex(pub_tx2)) } } + +func TestChainIdSigner_Equal(t *testing.T) { + + defaultChainID := big.NewInt(61) + + s := NewChainIdSigner(defaultChainID) + if s.chainId == nil || s.chainId.Cmp(new(big.Int)) == 0 || s.chainId.Cmp(big.NewInt(0)) == 0 || s.chainId.Cmp(defaultChainID) != 0 { + t.Errorf("unexpected: %v", s) + } + + s2Invalid0 := NewChainIdSigner(new(big.Int)) + if s.Equal(s2Invalid0) { + t.Errorf("unexpected: s: %v, s2: %v", s, s2Invalid0) + } + + s262 := NewChainIdSigner(big.NewInt(62)) + if s.Equal(s262) { + t.Errorf("unexpected: s: %v, s2: %v", s, s262) + } + + s261 := NewChainIdSigner(defaultChainID) + if !s.Equal(s261) { + t.Errorf("unexpected: s: %v, s2: %v", s, s261) + } +} From f1849111fa661d510cf51b22caa951f59592c462 Mon Sep 17 00:00:00 2001 From: "Mr. Is" Date: Tue, 9 May 2017 13:22:10 -0500 Subject: [PATCH 2/5] problem: ChainConfig.GetChainID untested and implementations assume non-zero-value solution: where config.ChainId required (for EIP155) double-check that ChainID is configured and not signing txs with zero-value. new tests passing. --- core/config_test.go | 15 +++++++++++++++ core/tx_pool.go | 7 ++++++- eth/api.go | 6 +++++- miner/worker.go | 7 ++++++- 4 files changed, 32 insertions(+), 3 deletions(-) diff --git a/core/config_test.go b/core/config_test.go index fd3fe026b..73053002f 100644 --- a/core/config_test.go +++ b/core/config_test.go @@ -432,6 +432,21 @@ func TestChainConfig_GetSigner(t *testing.T) { } +func TestChainConfig_GetChainID(t *testing.T) { + d := DefaultConfig + cid := d.GetChainID() + if cid.Cmp(new(big.Int)) == 0 { + t.Errorf("got: %v, want: %v", cid, DefaultChainConfigChainID) + } + + d = &ChainConfig{} // no chain id feature + cid = d.GetChainID() + if cid.Cmp(new(big.Int)) != 0 { + t.Errorf("got: %v, want: %v", cid, new(big.Int)) + } + +} + func makeOKSufficientChainConfig(dump *GenesisDump, config *ChainConfig) *SufficientChainConfig { // Setup. whole := &SufficientChainConfig{} diff --git a/core/tx_pool.go b/core/tx_pool.go index 95cced3a8..c994cc9ea 100644 --- a/core/tx_pool.go +++ b/core/tx_pool.go @@ -43,6 +43,7 @@ var ( ErrIntrinsicGas = errors.New("Intrinsic gas too low") ErrGasLimit = errors.New("Exceeds block gas limit") ErrNegativeValue = errors.New("Negative value") + ErrChainID = errors.New("Invalid (zero-value) chain id") ) const ( @@ -78,9 +79,13 @@ type TxPool struct { } func NewTxPool(config *ChainConfig, eventMux *event.TypeMux, currentStateFn stateFn, gasLimitFn func() *big.Int) *TxPool { + chainid := config.GetChainID() + if chainid.Cmp(new(big.Int)) == 0 { + panic(ErrChainID) + } pool := &TxPool{ config: config, - signer: types.NewChainIdSigner(config.GetChainID()), + signer: types.NewChainIdSigner(chainid), pending: make(map[common.Hash]*types.Transaction), queue: make(map[common.Address]map[common.Hash]*types.Transaction), eventMux: eventMux, diff --git a/eth/api.go b/eth/api.go index dd7a75811..9d6234740 100644 --- a/eth/api.go +++ b/eth/api.go @@ -825,7 +825,11 @@ func (s *PublicBlockChainAPI) rpcOutputBlock(b *types.Block, inclTx bool, fullTx if fullTx { formatTx = func(tx *types.Transaction) (interface{}, error) { if tx.Protected() { - tx.SetSigner(types.NewChainIdSigner(s.bc.Config().GetChainID())) + chainid := s.bc.Config().GetChainID() + if chainid.Cmp(new(big.Int)) == 0 { + return nil, errors.New("invalid chain configuration: chainid is zero-value") + } + tx.SetSigner(types.NewChainIdSigner(chainid)) } return newRPCTransaction(b, tx.Hash()) } diff --git a/miner/worker.go b/miner/worker.go index 88ed2d163..f51d0ec1e 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -24,6 +24,7 @@ import ( "sync/atomic" "time" + "errors" "github.com/ethereumproject/go-ethereum/accounts" "github.com/ethereumproject/go-ethereum/common" "github.com/ethereumproject/go-ethereum/core" @@ -356,9 +357,13 @@ func (self *worker) makeCurrent(parent *types.Block, header *types.Header) error if err != nil { return err } + chainid := self.config.GetChainID() + if chainid.Cmp(new(big.Int)) == 0 { + return errors.New("invalid chain configuration: chainid is zero-value") + } work := &Work{ config: self.config, - signer: types.NewChainIdSigner(self.config.GetChainID()), + signer: types.NewChainIdSigner(chainid), state: state, ancestors: set.New(), family: set.New(), From 334ec2965e543bd7381abd32e1108230ac31da3f Mon Sep 17 00:00:00 2001 From: "Mr. Is" Date: Tue, 9 May 2017 13:38:01 -0500 Subject: [PATCH 3/5] problem: new chainid implementation requires updated test config solution: include Diehard with features in /core test config --- core/block_validator_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/core/block_validator_test.go b/core/block_validator_test.go index 8ee925211..aeb25b0a2 100644 --- a/core/block_validator_test.go +++ b/core/block_validator_test.go @@ -51,6 +51,31 @@ func testChainConfig() *ChainConfig { }, }, }, + { + Name: "Diehard", + Block: big.NewInt(5), + Features: []*ForkFeature{ + { + ID: "eip155", + Options: ChainFeatureConfigOptions{ + "chainID": 62, + }, + }, + { // ecip1010 bomb delay + ID: "gastable", + Options: ChainFeatureConfigOptions{ + "type": "eip160", + }, + }, + { // ecip1010 bomb delay + ID: "difficulty", + Options: ChainFeatureConfigOptions{ + "type": "ecip1010", + "length": 2000000, + }, + }, + }, + }, }, } } From 3cd9c82b1602bbad18566fea8a70dcd0613f9f68 Mon Sep 17 00:00:00 2001 From: "Mr. Is" Date: Wed, 10 May 2017 07:01:24 -0500 Subject: [PATCH 4/5] problem: chainid should be allowed to be 0 (zero-value), but not nil solution: don't check against 0 in ChainConfig.GetChainID uses and simplify ChainIdSigner.Equal --- core/config_test.go | 9 ++++++++- core/tx_pool.go | 7 +------ core/types/transaction_signing.go | 5 +---- eth/api.go | 6 +----- miner/worker.go | 7 +------ 5 files changed, 12 insertions(+), 22 deletions(-) diff --git a/core/config_test.go b/core/config_test.go index 73053002f..51b34dc48 100644 --- a/core/config_test.go +++ b/core/config_test.go @@ -435,12 +435,19 @@ func TestChainConfig_GetSigner(t *testing.T) { func TestChainConfig_GetChainID(t *testing.T) { d := DefaultConfig cid := d.GetChainID() + // check is not zero if cid.Cmp(new(big.Int)) == 0 { t.Errorf("got: %v, want: %v", cid, DefaultChainConfigChainID) } + // check is expected default 61 + if cid.Cmp(DefaultChainConfigChainID) != 0 { + t.Errorf("got: %v, want: %v", cid, DefaultChainConfigChainID) + } - d = &ChainConfig{} // no chain id feature + // no chain id feature configured, should return zero-value (0) + d = &ChainConfig{} cid = d.GetChainID() + // check is zero if cid.Cmp(new(big.Int)) != 0 { t.Errorf("got: %v, want: %v", cid, new(big.Int)) } diff --git a/core/tx_pool.go b/core/tx_pool.go index c994cc9ea..95cced3a8 100644 --- a/core/tx_pool.go +++ b/core/tx_pool.go @@ -43,7 +43,6 @@ var ( ErrIntrinsicGas = errors.New("Intrinsic gas too low") ErrGasLimit = errors.New("Exceeds block gas limit") ErrNegativeValue = errors.New("Negative value") - ErrChainID = errors.New("Invalid (zero-value) chain id") ) const ( @@ -79,13 +78,9 @@ type TxPool struct { } func NewTxPool(config *ChainConfig, eventMux *event.TypeMux, currentStateFn stateFn, gasLimitFn func() *big.Int) *TxPool { - chainid := config.GetChainID() - if chainid.Cmp(new(big.Int)) == 0 { - panic(ErrChainID) - } pool := &TxPool{ config: config, - signer: types.NewChainIdSigner(chainid), + signer: types.NewChainIdSigner(config.GetChainID()), pending: make(map[common.Hash]*types.Transaction), queue: make(map[common.Address]map[common.Hash]*types.Transaction), eventMux: eventMux, diff --git a/core/types/transaction_signing.go b/core/types/transaction_signing.go index d5609ab15..f3c672607 100644 --- a/core/types/transaction_signing.go +++ b/core/types/transaction_signing.go @@ -137,10 +137,7 @@ func (s ChainIdSigner) Equal(s2 Signer) bool { if !ok { return false } - if other.chainId.Cmp(new(big.Int)) == 0 || other.chainId == nil { - return false - } - if s.chainId.Cmp(new(big.Int)) == 0 || s.chainId == nil { + if other.chainId == nil || s.chainId == nil { return false } diff --git a/eth/api.go b/eth/api.go index 9d6234740..dd7a75811 100644 --- a/eth/api.go +++ b/eth/api.go @@ -825,11 +825,7 @@ func (s *PublicBlockChainAPI) rpcOutputBlock(b *types.Block, inclTx bool, fullTx if fullTx { formatTx = func(tx *types.Transaction) (interface{}, error) { if tx.Protected() { - chainid := s.bc.Config().GetChainID() - if chainid.Cmp(new(big.Int)) == 0 { - return nil, errors.New("invalid chain configuration: chainid is zero-value") - } - tx.SetSigner(types.NewChainIdSigner(chainid)) + tx.SetSigner(types.NewChainIdSigner(s.bc.Config().GetChainID())) } return newRPCTransaction(b, tx.Hash()) } diff --git a/miner/worker.go b/miner/worker.go index f51d0ec1e..88ed2d163 100644 --- a/miner/worker.go +++ b/miner/worker.go @@ -24,7 +24,6 @@ import ( "sync/atomic" "time" - "errors" "github.com/ethereumproject/go-ethereum/accounts" "github.com/ethereumproject/go-ethereum/common" "github.com/ethereumproject/go-ethereum/core" @@ -357,13 +356,9 @@ func (self *worker) makeCurrent(parent *types.Block, header *types.Header) error if err != nil { return err } - chainid := self.config.GetChainID() - if chainid.Cmp(new(big.Int)) == 0 { - return errors.New("invalid chain configuration: chainid is zero-value") - } work := &Work{ config: self.config, - signer: types.NewChainIdSigner(chainid), + signer: types.NewChainIdSigner(self.config.GetChainID()), state: state, ancestors: set.New(), family: set.New(), From 68f193d107ca428eca6ec940f2c12d76df39417b Mon Sep 17 00:00:00 2001 From: "Mr. Is" Date: Fri, 12 May 2017 05:41:19 -0500 Subject: [PATCH 5/5] Fix merge redundancies and test both external chain configs. Both branches had made a TestChainConfig_GetChainID. --- core/config_test.go | 55 ++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/core/config_test.go b/core/config_test.go index 71af0de62..b6b5cf518 100644 --- a/core/config_test.go +++ b/core/config_test.go @@ -292,6 +292,7 @@ func TestChainConfig_GetFeature4_WorkForHighNumbers(t *testing.T) { } func TestChainConfig_GetChainID(t *testing.T) { + // Test default hardcoded configs. if DefaultConfig.GetChainID().Cmp(DefaultChainConfigChainID) != 0 { t.Error("got: %v, want: %v", DefaultConfig.GetChainID(), DefaultTestnetChainConfigChainID) } @@ -299,17 +300,31 @@ func TestChainConfig_GetChainID(t *testing.T) { t.Error("got: %v, want: %v", TestConfig.GetChainID(), DefaultTestnetChainConfigChainID) } - // Test parsing default external config. - p, e := filepath.Abs("../cmd/geth/config/mainnet.json") - if e != nil { - t.Errorf("filepath err: %v", e) + // If no chainID (config is empty) returns 0. + c := &ChainConfig{} + cid := c.GetChainID() + // check is zero + if cid.Cmp(new(big.Int)) != 0 { + t.Errorf("got: %v, want: %v", cid, new(big.Int)) } - extConfig, err := ReadExternalChainConfig(p) - if err != nil { - t.Errorf("could not find file: %v", err) + + // Test parsing default external mainnet config. + cases := map[string]*big.Int{ + "../cmd/geth/config/mainnet.json": DefaultChainConfigChainID, + "../cmd/geth/config/testnet.json": DefaultTestnetChainConfigChainID, } - if extConfig.ChainConfig.GetChainID().Cmp(big.NewInt(61)) != 0 { - t.Error("found 0 chainid for eip155") + for extConfigPath, wantInt := range cases { + p, e := filepath.Abs(extConfigPath) + if e != nil { + t.Errorf("filepath err: %v", e) + } + extConfig, err := ReadExternalChainConfig(p) + if err != nil { + t.Errorf("could not find file: %v", err) + } + if extConfig.ChainConfig.GetChainID().Cmp(wantInt) != 0 { + t.Error("got: %v, want: %v", extConfig.ChainConfig.GetChainID(), wantInt) + } } } @@ -491,28 +506,6 @@ func TestChainConfig_GetSigner(t *testing.T) { } -func TestChainConfig_GetChainID(t *testing.T) { - d := DefaultConfig - cid := d.GetChainID() - // check is not zero - if cid.Cmp(new(big.Int)) == 0 { - t.Errorf("got: %v, want: %v", cid, DefaultChainConfigChainID) - } - // check is expected default 61 - if cid.Cmp(DefaultChainConfigChainID) != 0 { - t.Errorf("got: %v, want: %v", cid, DefaultChainConfigChainID) - } - - // no chain id feature configured, should return zero-value (0) - d = &ChainConfig{} - cid = d.GetChainID() - // check is zero - if cid.Cmp(new(big.Int)) != 0 { - t.Errorf("got: %v, want: %v", cid, new(big.Int)) - } - -} - func makeOKSufficientChainConfig(dump *GenesisDump, config *ChainConfig) *SufficientChainConfig { // Setup. whole := &SufficientChainConfig{}