From e3b38b1bfd4d021201328da5f52ec0704f10fac9 Mon Sep 17 00:00:00 2001
From: Jason Yellick <jyellick@us.ibm.com>
Date: Mon, 24 Feb 2020 14:46:42 -0500
Subject: [PATCH] Fix nil dereference in etcdraft config parsing (#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>
---
 orderer/consensus/etcdraft/util.go      |  6 +++++-
 orderer/consensus/etcdraft/util_test.go | 26 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/orderer/consensus/etcdraft/util.go b/orderer/consensus/etcdraft/util.go
index 6b366173e44..36a2013d231 100644
--- 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
index 70e8c77978d..e8acce39d43 100644
--- 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" +