server: fix fsm.keepaliveTicker race

When fsm state goes to idle from established, fsm.keepaliveTicker is
set to nil. This can happen before <-h.t.Dying() case in
sendMessageloop().

This removes fsm.keepaliveTicker. Ticker is created locally. With
this, a keepalive message could be sent from openconfirm to
established with a shorter interval. But it should not break anything.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

2 files changed, 24 insertions, 28 deletions

diff --git a/server/fsm.go b/server/fsm.go
index 749c925b..c25fd1f3 100644
--- a/server/fsm.go
+++ b/server/fsm.go
@@ -68,7 +68,6 @@ type FSM struct {
 	t                  tomb.Tomb
 	globalConfig       *config.Global
 	peerConfig         *config.Neighbor
-	keepaliveTicker    *time.Ticker
 	state              bgp.FSMState
 	conn               net.Conn
 	connCh             chan net.Conn
@@ -280,13 +279,6 @@ func NewFSMHandler(fsm *FSM, incoming chan *fsmMsg, outgoing chan *bgp.BGPMessag
 func (h *FSMHandler) idle() bgp.FSMState {
 	fsm := h.fsm
 
-	if fsm.keepaliveTicker != nil {
-		if fsm.negotiatedHoldTime != 0 {
-			fsm.keepaliveTicker.Stop()
-		}
-		fsm.keepaliveTicker = nil
-	}
-
 	idleHoldTimer := time.NewTimer(time.Second * time.Duration(fsm.idleHoldTime))
 	for {
 		select {
@@ -575,9 +567,23 @@ func (h *FSMHandler) opensent() bgp.FSMState {
 	}
 }
 
+func keepaliveTicker(fsm *FSM) *time.Ticker {
+	if fsm.negotiatedHoldTime == 0 {
+		return &time.Ticker{}
+	}
+	sec := time.Second * time.Duration(fsm.peerConfig.Timers.KeepaliveInterval)
+	if fsm.negotiatedHoldTime < fsm.peerConfig.Timers.HoldTime {
+		sec = time.Second * time.Duration(fsm.negotiatedHoldTime) / 3
+	}
+	if sec == 0 {
+		sec = 1
+	}
+	return time.NewTicker(sec)
+}
+
 func (h *FSMHandler) openconfirm() bgp.FSMState {
 	fsm := h.fsm
-
+	ticker := keepaliveTicker(fsm)
 	h.msgCh = make(chan *fsmMsg)
 	h.conn = fsm.conn
 
@@ -585,15 +591,8 @@ func (h *FSMHandler) openconfirm() bgp.FSMState {
 
 	var holdTimer *time.Timer
 	if fsm.negotiatedHoldTime == 0 {
-		fsm.keepaliveTicker = &time.Ticker{}
 		holdTimer = &time.Timer{}
 	} else {
-		sec := time.Second * time.Duration(fsm.peerConfig.Timers.KeepaliveInterval)
-		if fsm.negotiatedHoldTime < fsm.peerConfig.Timers.HoldTime {
-			sec = time.Second * time.Duration(fsm.negotiatedHoldTime) / 3
-		}
-		fsm.keepaliveTicker = time.NewTicker(sec)
-
 		// RFC 4271 P.65
 		// sets the HoldTimer according to the negotiated value
 		holdTimer = time.NewTimer(time.Second * time.Duration(fsm.negotiatedHoldTime))
@@ -613,7 +612,7 @@ func (h *FSMHandler) openconfirm() bgp.FSMState {
 				"Topic": "Peer",
 				"Key":   fsm.peerConfig.NeighborAddress,
 			}).Warn("Closed an accepted connection")
-		case <-fsm.keepaliveTicker.C:
+		case <-ticker.C:
 			m := bgp.NewBGPKeepAliveMessage()
 			b, _ := m.Serialize()
 			// TODO: check error
@@ -676,6 +675,7 @@ func (h *FSMHandler) openconfirm() bgp.FSMState {
 func (h *FSMHandler) sendMessageloop() error {
 	conn := h.conn
 	fsm := h.fsm
+	ticker := keepaliveTicker(fsm)
 	send := func(m *bgp.BGPMessage) error {
 		b, err := m.Serialize()
 		if err != nil {
@@ -748,7 +748,7 @@ func (h *FSMHandler) sendMessageloop() error {
 			if err := send(m); err != nil {
 				return nil
 			}
-		case <-fsm.keepaliveTicker.C:
+		case <-ticker.C:
 			if err := send(bgp.NewBGPKeepAliveMessage()); err != nil {
 				return nil
 			}
diff --git a/server/fsm_test.go b/server/fsm_test.go
index 6f18014d..ff758253 100644
--- a/server/fsm_test.go
+++ b/server/fsm_test.go
@@ -163,9 +163,8 @@ func TestFSMHandlerOpensent_HoldTimerExpired(t *testing.T) {
 	// push mock connection
 	p.fsm.conn = m
 
-	// set up keepalive ticker
-	sec := time.Second * 1
-	p.fsm.keepaliveTicker = time.NewTicker(sec)
+	// set keepalive ticker
+	p.fsm.negotiatedHoldTime = 3
 
 	// set holdtime
 	p.fsm.opensentHoldTime = 2
@@ -213,9 +212,8 @@ func TestFSMHandlerEstablish_HoldTimerExpired(t *testing.T) {
 	// push mock connection
 	p.fsm.conn = m
 
-	// set up keepalive ticker
-	sec := time.Second * 1
-	p.fsm.keepaliveTicker = time.NewTicker(sec)
+	// set keepalive ticker
+	p.fsm.negotiatedHoldTime = 3
 
 	msg := keepalive()
 	header, _ := msg.Header.Serialize()
@@ -273,10 +271,8 @@ func TestFSMHandlerEstablished_HoldtimeZero(t *testing.T) {
 	// push mock connection
 	p.fsm.conn = m
 
-	// set up keepalive ticker
-	sec := time.Second * 1
-	p.fsm.keepaliveTicker = time.NewTicker(sec)
-	p.fsm.keepaliveTicker.Stop()
+	// set keepalive ticker
+	p.fsm.negotiatedHoldTime = 3
 
 	// set holdtime
 	p.fsm.negotiatedHoldTime = 0