Fix nil dereference in etcdraft config parsing (hyperledger#724)

The etcdraft config parsing code currently checks that the consensus metadata is not nil, but it fails to check that the options are not nil. This commit simply adds the additional check and backfills some testing around this validation function. Signed-off-by: Jason Yellick <jyellick@us.ibm.com>
C0rWin · Sep 15, 2020 · e3b38b1 · e3b38b1
1 parent 95e1482
commit e3b38b1
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 1 deletion.
diff --git a/orderer/consensus/etcdraft/util.go b/orderer/consensus/etcdraft/util.go
@@ -385,6 +385,10 @@ func CheckConfigMetadata(metadata *etcdraft.ConfigMetadata) error {
 		return errors.Errorf("nil Raft config metadata")
 	}
 
+	if metadata.Options == nil {
+		return errors.Errorf("nil Raft config metadata options")
+	}
+
 	if metadata.Options.HeartbeatTick == 0 ||
 		metadata.Options.ElectionTick == 0 ||
 		metadata.Options.MaxInflightBlocks == 0 {
@@ -396,7 +400,7 @@ func CheckConfigMetadata(metadata *etcdraft.ConfigMetadata) error {
 	// check Raft options
 	if metadata.Options.ElectionTick <= metadata.Options.HeartbeatTick {
 		return errors.Errorf("ElectionTick (%d) must be greater than HeartbeatTick (%d)",
-			metadata.Options.HeartbeatTick, metadata.Options.HeartbeatTick)
+			metadata.Options.ElectionTick, metadata.Options.HeartbeatTick)
 	}
 
 	if d, err := time.ParseDuration(metadata.Options.TickInterval); err != nil {

diff --git a/orderer/consensus/etcdraft/util_test.go b/orderer/consensus/etcdraft/util_test.go
@@ -25,6 +25,7 @@ import (
 	"github.com/hyperledger/fabric/orderer/consensus"
 	"github.com/hyperledger/fabric/orderer/mocks/common/multichannel"
 	"github.com/hyperledger/fabric/protos/common"
+	"github.com/hyperledger/fabric/protos/orderer/etcdraft"
 	"github.com/hyperledger/fabric/protos/utils"
 	"github.com/onsi/gomega"
 	"github.com/pkg/errors"
@@ -34,6 +35,31 @@ import (
 	"go.uber.org/zap/zapcore"
 )
 
+func TestCheckConfigMetadata(t *testing.T) {
+	tests := []struct {
+		metadata *etcdraft.ConfigMetadata
+		err      string
+	}{
+		{nil, "nil Raft config metadata"},
+		{&etcdraft.ConfigMetadata{}, "nil Raft config metadata options"},
+		{&etcdraft.ConfigMetadata{Options: &etcdraft.Options{}}, "none of HeartbeatTick (0), ElectionTick (0) and MaxInflightBlocks (0) can be zero"},
+		{&etcdraft.ConfigMetadata{Options: &etcdraft.Options{HeartbeatTick: 2, ElectionTick: 1, MaxInflightBlocks: 1}}, "ElectionTick (1) must be greater than HeartbeatTick (2)"},
+		{&etcdraft.ConfigMetadata{Options: &etcdraft.Options{HeartbeatTick: 1, ElectionTick: 2, MaxInflightBlocks: 1, TickInterval: "q"}}, "failed to parse TickInterval (q) to time duration: time: invalid duration q"},
+		{&etcdraft.ConfigMetadata{Options: &etcdraft.Options{HeartbeatTick: 1, ElectionTick: 2, MaxInflightBlocks: 1, TickInterval: "0"}}, "TickInterval cannot be zero"},
+		{&etcdraft.ConfigMetadata{Options: &etcdraft.Options{HeartbeatTick: 1, ElectionTick: 2, MaxInflightBlocks: 1, TickInterval: "1s"}}, "empty consenter set"},
+	}
+
+	for _, tc := range tests {
+		err := CheckConfigMetadata(tc.metadata)
+		if tc.err == "" {
+			assert.NoError(t, err)
+		} else {
+			assert.EqualError(t, err, tc.err)
+		}
+	}
+
+}
+
 func TestIsConsenterOfChannel(t *testing.T) {
 	certInsideConfigBlock, err := base64.StdEncoding.DecodeString("LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUNmekNDQWlhZ0F3SUJBZ0l" +
 		"SQUo4bjFLYTVzS1ZaTXRMTHJ1dldERDB3Q2dZSUtvWkl6ajBFQXdJd2JERUwKTUFrR0ExVUVCaE1DVlZNeEV6QVJCZ05WQkFnVENrTmhiR" +