[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 <[email protected]>

Author: Jason Yellick

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 {

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")
+	}
+}

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
 }
 

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)