Merge pull request #520 from dedis/recover_panic_1725

Recover panic 1725

overlay.go

@@ -167,11 +167,17 @@ func (o *Overlay) TransmitMsg(onetMsg *ProtocolMsg, io MessageProxy) error {
 			return nil
 		}
 		go func() {
+			defer func() {
+				if r := recover(); r != nil {
+					svc := ServiceFactory.Name(tni.Token().ServiceID)
+					log.Errorf("Panic in %v %s.Dispatch(): %v", svc, o.server.ServerIdentity, r)
+				}
+			}()
+
 			err := pi.Dispatch()
 			if err != nil {
 				svc := ServiceFactory.Name(tni.Token().ServiceID)
-				log.Errorf("%v %s.Dispatch() returned error %s",
-					o.server.ServerIdentity, svc, err)
+				log.Errorf("%v %s.Dispatch() returned error %s", o.server.ServerIdentity, svc, err)
 			}
 		}()
 		if err := o.RegisterProtocolInstance(pi); err != nil {
@@ -206,6 +212,8 @@ func (o *Overlay) addPendingTreeMarshal(tm *TreeMarshal) {
 // some pending ProtocolMessage messages using this tree. If there are, we can
 // make an instance of a protocolinstance and give it the message.
 func (o *Overlay) checkPendingMessages(t *Tree) {
+	// This goroutine has no recover because the underlying code should never panic
+	// and TransmitMsg does its own recovering
 	go func() {
 		o.pendingMsgLock.Lock()
 
@@ -552,6 +560,12 @@ func (o *Overlay) CreateProtocol(name string, t *Tree, sid ServiceID) (ProtocolI
 		return nil, err
 	}
 	go func() {
+		defer func() {
+			if r := recover(); r != nil {
+				log.Errorf("Panic in %s.Dispatch(): %v", name, r)
+			}
+		}()
+
 		err := pi.Dispatch()
 		if err != nil {
 			log.Errorf("%s.Dispatch() created in service %s returned error %s",
@@ -568,6 +582,12 @@ func (o *Overlay) StartProtocol(name string, t *Tree, sid ServiceID) (ProtocolIn
 		return nil, err
 	}
 	go func() {
+		defer func() {
+			if r := recover(); r != nil {
+				log.Errorf("Panic in %s.Start(): %v", name, r)
+			}
+		}()
+
 		err := pi.Start()
 		if err != nil {
 			log.Error("Error while starting:", err)

overlay_test.go

@@ -134,6 +134,77 @@ func TestOverlayDone(t *testing.T) {
 	}
 }
 
+type protocolCatastrophic struct {
+	*TreeNodeInstance
+
+	ChannelMsg chan WrapDummyMsg
+
+	done chan bool
+}
+
+func (po *protocolCatastrophic) Start() error {
+	panic("start panic")
+}
+
+func (po *protocolCatastrophic) Dispatch() error {
+	if !po.IsRoot() {
+		<-po.ChannelMsg
+
+		po.SendToParent(&DummyMsg{})
+
+		po.Done()
+		panic("dispatch panic")
+	}
+
+	err := po.SendToChildren(&DummyMsg{})
+	if err != nil {
+		return err
+	}
+
+	<-po.ChannelMsg
+	<-po.ChannelMsg
+	po.done <- true
+
+	po.Done()
+	panic("root dispatch panic")
+}
+
+// TestOverlayCatastrophicFailure checks if a panic during a protocol could
+// cause the server to crash
+func TestOverlayCatastrophicFailure(t *testing.T) {
+	log.OutputToBuf()
+	defer log.OutputToOs()
+
+	fn := func(n *TreeNodeInstance) (ProtocolInstance, error) {
+		ps := protocolCatastrophic{
+			TreeNodeInstance: n,
+			done:             make(chan bool),
+		}
+
+		err := ps.RegisterChannel(&ps.ChannelMsg)
+
+		return &ps, err
+	}
+	GlobalProtocolRegister("ProtocolCatastrophic", fn)
+	local := NewLocalTest(tSuite)
+	defer local.CloseAll()
+
+	h, _, tree := local.GenTree(3, true)
+	h1 := h[0]
+	pi, err := h1.StartProtocol("ProtocolCatastrophic", tree)
+	assert.NoError(t, err)
+
+	<-pi.(*protocolCatastrophic).done
+
+	// can't have a synchronisation and a panic so we wait for the panic to be handled
+	time.Sleep(1 * time.Second)
+
+	stderr := log.GetStdErr()
+	assert.Contains(t, stderr, "Start(): start panic")
+	assert.Contains(t, stderr, "Dispatch(): dispatch panic")
+	assert.Contains(t, stderr, "Dispatch(): root dispatch panic")
+}
+
 // Test when a peer receives a New Roster, it can create the trees that are
 // waiting on this specific entitiy list, to be constructed.
 func TestOverlayPendingTreeMarshal(t *testing.T) {

tree.go

@@ -419,6 +419,13 @@ func NewRoster(ids []*network.ServerIdentity) *Roster {
 		if err != nil {
 			log.Error(err)
 		}
+
+		for _, srvid := range id.ServiceIdentities {
+			_, err = srvid.Public.MarshalTo(h)
+			if err != nil {
+				log.Error(err)
+			}
+		}
 	}
 
 	r := &Roster{

tree_test.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.dedis.ch/kyber/v3/suites"
 	"go.dedis.ch/kyber/v3/util/key"
 	"go.dedis.ch/onet/v3/log"
 	"go.dedis.ch/onet/v3/network"
@@ -431,6 +432,22 @@ func TestTreeNode_SubtreeCount(t *testing.T) {
 	}
 }
 
+func TestRoster_ID(t *testing.T) {
+	names := genLocalhostPeerNames(10, 2000)
+	ro := genRoster(tSuite, names)
+	ro2 := NewRoster(ro.List)
+
+	assert.True(t, ro.ID.Equal(ro2.ID))
+
+	tt := []*network.ServerIdentity{}
+	for _, id := range ro.List {
+		tt = append(tt, network.NewServerIdentity(id.Public, id.Address))
+	}
+
+	ro3 := NewRoster(tt)
+	assert.False(t, ro3.ID.Equal(ro.ID))
+}
+
 func TestRoster_GenerateNaryTree(t *testing.T) {
 	names := genLocalhostPeerNames(10, 2000)
 	peerList := genRoster(tSuite, names)
@@ -666,15 +683,24 @@ func genLocalPeerName(nbrLocal, nbrPort int) []network.Address {
 }
 
 // genRoster generates a Roster out of names
-func genRoster(suite network.Suite, names []network.Address) *Roster {
+func genRoster(suite suites.Suite, names []network.Address) *Roster {
 	var ids []*network.ServerIdentity
 	for _, n := range names {
 		kp := key.NewKeyPair(suite)
-		ids = append(ids, network.NewServerIdentity(kp.Public, n))
+		srvid := network.NewServerIdentity(kp.Public, n)
+		srvid.ServiceIdentities = []network.ServiceIdentity{genServiceIdentity(suite)}
+
+		ids = append(ids, srvid)
 	}
 	return NewRoster(ids)
 }
 
+func genServiceIdentity(suite suites.Suite) network.ServiceIdentity {
+	kp := key.NewKeyPair(suite)
+
+	return network.NewServiceIdentityFromPair("ServiceTest", suite, kp)
+}
+
 func genLocalTree(count, port int) (*Tree, *Roster) {
 	names := genLocalhostPeerNames(count, port)
 	peerList := genRoster(tSuite, names)