[FAB-1765] Fix orderer crash

https://jira.hyperledger.org/browse/FAB-1765 When the orderer is initialized with an empty slice for ChainCreationPolicyNames, no chain is initialized as the system chain. This does not cause a panic, and leads to a nil pointer dereference when a new chain proposal tries to reference the system chain. This is a byproduct of the fact that empty slices marshal to nil under protobuf, so the sharedconfig code was setting the ChainCreationPolicyNames to nil rather than an empty slice. The system chain was detected by looking for a non-nil ChainCreationPolicyNames. Change-Id: I4c8bf40e89dc73ba4ee27646b3f66c65ecb22d38 Signed-off-by: Jason Yellick <jyellick@us.ibm.com>
hyperledger · Jan 19, 2017 · a05cf54 · a05cf54
1 parent 7e52b66
commit a05cf54
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 11 deletions.
diff --git a/orderer/common/sharedconfig/sharedconfig.go b/orderer/common/sharedconfig/sharedconfig.go
@@ -86,13 +86,13 @@ type Manager interface {
 }
 
 type ordererConfig struct {
-	consensusType         string
-	batchSize             *ab.BatchSize
-	batchTimeout          time.Duration
-	chainCreationPolicies []string
-	kafkaBrokers          []string
-	ingressPolicyNames    []string
-	egressPolicyNames     []string
+	consensusType            string
+	batchSize                *ab.BatchSize
+	batchTimeout             time.Duration
+	chainCreationPolicyNames []string
+	kafkaBrokers             []string
+	ingressPolicyNames       []string
+	egressPolicyNames        []string
 }
 
 // ManagerImpl is an implementation of Manager and configtx.ConfigHandler
@@ -127,7 +127,7 @@ func (pm *ManagerImpl) BatchTimeout() time.Duration {
 // ChainCreationPolicyNames returns the policy names which are allowed for chain creation
 // This field is only set for the system ordering chain
 func (pm *ManagerImpl) ChainCreationPolicyNames() []string {
-	return pm.config.chainCreationPolicies
+	return pm.config.chainCreationPolicyNames
 }
 
 // KafkaBrokers returns the addresses (IP:port notation) of a set of "bootstrap"
@@ -225,11 +225,17 @@ func (pm *ManagerImpl) ProposeConfig(configItem *cb.ConfigurationItem) error {
 		}
 		pm.pendingConfig.batchTimeout = timeoutValue
 	case ChainCreationPolicyNamesKey:
-		chainCreationPolicies := &ab.ChainCreationPolicyNames{}
-		if err := proto.Unmarshal(configItem.Value, chainCreationPolicies); err != nil {
+		chainCreationPolicyNames := &ab.ChainCreationPolicyNames{}
+		if err := proto.Unmarshal(configItem.Value, chainCreationPolicyNames); err != nil {
 			return fmt.Errorf("Unmarshaling error for ChainCreator: %s", err)
 		}
-		pm.pendingConfig.chainCreationPolicies = chainCreationPolicies.Names
+		if chainCreationPolicyNames.Names == nil {
+			// Proto unmarshals empty slices to nil, but this poses a problem for us in detecting the system chain
+			// if it does not set this value, so explicitly set the policies to the empty string slice, if it is set
+			pm.pendingConfig.chainCreationPolicyNames = []string{}
+		} else {
+			pm.pendingConfig.chainCreationPolicyNames = chainCreationPolicyNames.Names
+		}
 	case IngressPolicyNamesKey:
 		ingressPolicyNames := &ab.IngressPolicyNames{}
 		if err := proto.Unmarshal(configItem.Value, ingressPolicyNames); err != nil {

diff --git a/orderer/common/sharedconfig/sharedconfig_test.go b/orderer/common/sharedconfig/sharedconfig_test.go
@@ -319,3 +319,20 @@ func TestChainCreationPolicyNames(t *testing.T) {
 	m := NewManagerImpl()
 	testPolicyNames(m, ChainCreationPolicyNamesKey, TemplateChainCreationPolicyNames, m.ChainCreationPolicyNames, t)
 }
+
+func TestEmptyChainCreationPolicyNames(t *testing.T) {
+	m := NewManagerImpl()
+
+	m.BeginConfig()
+
+	err := m.ProposeConfig(TemplateChainCreationPolicyNames(nil))
+	if err != nil {
+		t.Fatalf("Error applying valid config: %s", err)
+	}
+
+	m.CommitConfig()
+
+	if m.ChainCreationPolicyNames() == nil {
+		t.Fatalf("Should have gotten back empty slice, not nil")
+	}
+}
diff --git a/orderer/multichain/manager.go b/orderer/multichain/manager.go
@@ -142,6 +142,10 @@ func NewManagerImpl(ledgerFactory ordererledger.Factory, consenters map[string]C
 
 	}
 
+	if ml.sysChain == nil {
+		logger.Panicf("No system chain found")
+	}
+
 	return ml
 }
 

diff --git a/orderer/multichain/manager_test.go b/orderer/multichain/manager_test.go
@@ -102,6 +102,22 @@ func TestGetConfigTxFailure(t *testing.T) {
 
 }
 
+// This test essentially brings the entire system up and is ultimately what main.go will replicate
+func TestNoSystemChain(t *testing.T) {
+	defer func() {
+		if recover() == nil {
+			t.Fatalf("Should have panicked when starting without a system chain")
+		}
+	}()
+
+	lf := ramledger.New(10)
+
+	consenters := make(map[string]Consenter)
+	consenters[conf.Genesis.OrdererType] = &mockConsenter{}
+
+	NewManagerImpl(lf, consenters)
+}
+
 // This test essentially brings the entire system up and is ultimately what main.go will replicate
 func TestManagerImpl(t *testing.T) {
 	lf, rl := NewRAMLedgerAndFactory(10)
-Original file line number
+Diff line change
@@ Expand Up @@
     	}
+    	if ml.sysChain == nil {
+    		logger.Panicf("No system chain found")
+    	}
     	return ml
     }
@@ Expand Down @@