Skip to content

Commit

Permalink
[FAB-3450] Prevent panic on msg signing
Browse files Browse the repository at this point in the history 
This addresses a missed nil check in the code that
https://gerrit.hyperledger.org/r/#/c/10259/ introduced.

Change-Id: Ie6a854ce85964d0252f9dcca95374a3bba6d1155
Signed-off-by: Yacov Manevich <yacovm@il.ibm.com>
  • Loading branch information
@yacovm
yacovm committed Jun 12, 2017
1 parent 307c903 commit 93b9076
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 21 deletions.
48 changes: 35 additions & 13 deletions gossip/discovery/discovery_impl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package discovery

import (
"bytes"
"errors"
"fmt"
"math"
"strconv"
Expand Down Expand Up @@ -172,7 +173,12 @@ func (d *gossipDiscoveryImpl) Connect(member NetworkMember, id identifier) {
Endpoint: member.Endpoint,
PKIid: id.ID,
}
req, err := d.createMembershipRequest(id.SelfOrg).NoopSign()
m, err := d.createMembershipRequest(id.SelfOrg)
if err != nil {
d.logger.Warning("Failed creating membership request:", err)
continue
}
req, err := m.NoopSign()
if err != nil {
d.logger.Warning("Failed creating SignedGossipMessage:", err)
continue
Expand Down Expand Up @@ -234,7 +240,12 @@ func (d *gossipDiscoveryImpl) InitiateSync(peerNum int) {
return
}
var peers2SendTo []*NetworkMember
memReq, err := d.createMembershipRequest(true).NoopSign()
m, err := d.createMembershipRequest(true)
if err != nil {
d.logger.Warning("Failed creating membership request:", err)
return
}
memReq, err := m.NoopSign()
if err != nil {
d.logger.Warning("Failed creating SignedGossipMessage:", err)
return
Expand Down Expand Up @@ -409,8 +420,9 @@ func (d *gossipDiscoveryImpl) sendMemResponse(targetMember *proto.Member, intern
InternalEndpoint: internalEndpoint,
}

aliveMsg := d.createAliveMessage(true)
if aliveMsg == nil {
aliveMsg, err := d.createAliveMessage(true)
if err != nil {
d.logger.Warning("Failed creating alive message:", err)
return
}
memResp := d.createMembershipResponse(aliveMsg, targetPeer)
Expand Down Expand Up @@ -607,17 +619,26 @@ func (d *gossipDiscoveryImpl) periodicalReconnectToDead() {
}

func (d *gossipDiscoveryImpl) sendMembershipRequest(member *NetworkMember, includeInternalEndpoint bool) {
req, err := d.createMembershipRequest(includeInternalEndpoint).NoopSign()
m, err := d.createMembershipRequest(includeInternalEndpoint)
if err != nil {
d.logger.Warning("Failed creating membership request:", err)
return
}
req, err := m.NoopSign()
if err != nil {
d.logger.Error("Failed creating SignedGossipMessage:", err)
return
}
d.comm.SendToPeer(member, req)
}

func (d *gossipDiscoveryImpl) createMembershipRequest(includeInternalEndpoint bool) *proto.GossipMessage {
func (d *gossipDiscoveryImpl) createMembershipRequest(includeInternalEndpoint bool) (*proto.GossipMessage, error) {
am, err := d.createAliveMessage(includeInternalEndpoint)
if err != nil {
return nil, err
}
req := &proto.MembershipRequest{
SelfInformation: d.createAliveMessage(includeInternalEndpoint).Envelope,
SelfInformation: am.Envelope,
// TODO: sending the known peers is not secure because the remote peer might shouldn't know
// TODO: about the known peers. I'm deprecating this until a secure mechanism will be implemented.
// TODO: See FAB-2570 for tracking this issue.
Expand All @@ -629,7 +650,7 @@ func (d *gossipDiscoveryImpl) createMembershipRequest(includeInternalEndpoint bo
Content: &proto.GossipMessage_MemReq{
MemReq: req,
},
}
}, nil
}

func (d *gossipDiscoveryImpl) copyLastSeen(lastSeenMap map[string]*timestamp) []NetworkMember {
Expand Down Expand Up @@ -711,15 +732,16 @@ func (d *gossipDiscoveryImpl) periodicalSendAlive() {
for !d.toDie() {
d.logger.Debug("Sleeping", getAliveTimeInterval())
time.Sleep(getAliveTimeInterval())
msg := d.createAliveMessage(true)
if msg == nil {
msg, err := d.createAliveMessage(true)
if err != nil {
d.logger.Warning("Failed creating alive message:", err)
return
}
d.comm.Gossip(msg)
}
}

func (d *gossipDiscoveryImpl) createAliveMessage(includeInternalEndpoint bool) *proto.SignedGossipMessage {
func (d *gossipDiscoveryImpl) createAliveMessage(includeInternalEndpoint bool) (*proto.SignedGossipMessage, error) {
d.lock.Lock()
d.seqNum++
seqNum := d.seqNum
Expand Down Expand Up @@ -750,7 +772,7 @@ func (d *gossipDiscoveryImpl) createAliveMessage(includeInternalEndpoint bool) *

envp := d.crypt.SignMessage(msg2Gossip, internalEndpoint)
if envp == nil {
return nil
return nil, errors.New("Failed signing message")
}
signedMsg := &proto.SignedGossipMessage{
GossipMessage: msg2Gossip,
Expand All @@ -761,7 +783,7 @@ func (d *gossipDiscoveryImpl) createAliveMessage(includeInternalEndpoint bool) *
signedMsg.Envelope.SecretEnvelope = nil
}

return signedMsg
return signedMsg, nil
}

func (d *gossipDiscoveryImpl) learnExistingMembers(aliveArr []*proto.SignedGossipMessage) {
Expand Down
18 changes: 10 additions & 8 deletions gossip/discovery/discovery_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -413,9 +413,11 @@ func TestConnect(t *testing.T) {
waitUntilOrFail(t, fullMembership)

discInst := instances[rand.Intn(len(instances))].Discovery.(*gossipDiscoveryImpl)
am, _ := discInst.createMembershipRequest(true).GetMemReq().SelfInformation.ToGossipMessage()
mr, _ := discInst.createMembershipRequest(true)
am, _ := mr.GetMemReq().SelfInformation.ToGossipMessage()
assert.NotNil(t, am.SecretEnvelope)
am, _ = discInst.createMembershipRequest(false).GetMemReq().SelfInformation.ToGossipMessage()
mr2, _ := discInst.createMembershipRequest(false)
am, _ = mr2.GetMemReq().SelfInformation.ToGossipMessage()
assert.Nil(t, am.SecretEnvelope)
stopInstances(t, instances)
}
Expand Down Expand Up @@ -920,13 +922,13 @@ func TestMsgStoreExpirationWithMembershipMessages(t *testing.T) {

// Creating MembershipRequest messages
for i := 0; i < peersNum; i++ {
memReqMsg := instances[i].discoveryImpl().createMembershipRequest(true)
memReqMsg, _ := instances[i].discoveryImpl().createMembershipRequest(true)
sMsg, _ := memReqMsg.NoopSign()
memReqMsgs = append(memReqMsgs, sMsg)
}
// Creating Alive messages
for i := 0; i < peersNum; i++ {
aliveMsg := instances[i].discoveryImpl().createAliveMessage(true)
aliveMsg, _ := instances[i].discoveryImpl().createAliveMessage(true)
aliveMsgs = append(aliveMsgs, aliveMsg)
}

Expand Down Expand Up @@ -989,15 +991,15 @@ func TestMsgStoreExpirationWithMembershipMessages(t *testing.T) {
return k == i
},
func(k int) {
aliveMsg := instances[k].discoveryImpl().createAliveMessage(true)
aliveMsg, _ := instances[k].discoveryImpl().createAliveMessage(true)
memResp := instances[k].discoveryImpl().createMembershipResponse(aliveMsg, peerToResponse)
memRespMsgs[i] = append(memRespMsgs[i], memResp)
})
}

// Re-creating Alive msgs with highest seq_num, to make sure Alive msgs in memReq and memResp are older
for i := 0; i < peersNum; i++ {
aliveMsg := instances[i].discoveryImpl().createAliveMessage(true)
aliveMsg, _ := instances[i].discoveryImpl().createAliveMessage(true)
newAliveMsgs = append(newAliveMsgs, aliveMsg)
}

Expand Down Expand Up @@ -1104,13 +1106,13 @@ func TestAliveMsgStore(t *testing.T) {

// Creating MembershipRequest messages
for i := 0; i < peersNum; i++ {
memReqMsg := instances[i].discoveryImpl().createMembershipRequest(true)
memReqMsg, _ := instances[i].discoveryImpl().createMembershipRequest(true)
sMsg, _ := memReqMsg.NoopSign()
memReqMsgs = append(memReqMsgs, sMsg)
}
// Creating Alive messages
for i := 0; i < peersNum; i++ {
aliveMsg := instances[i].discoveryImpl().createAliveMessage(true)
aliveMsg, _ := instances[i].discoveryImpl().createAliveMessage(true)
aliveMsgs = append(aliveMsgs, aliveMsg)
}

Expand Down

0 comments on commit 93b9076

Please sign in to comment.