Fix bug in NOTIFICATION handling before FSM reaches Established state.ex07

Change removes the setting of session->active to false which was being
done before a NOTIFICATION message is sent.  This flag should remain
true when the session is not being stopped -- which is the case if
a NOTIFICATION is sent before Established state is reached.

Effect of this bug was to trip up the accept() side of the session,
bringing bgpd down on an assert().

This bug may be triggered by a peer that accepts a connection and then
remains silent, for whatever reason -- causing bgpd to issue an
HoldTimer Expired NOTIFICATION.

Version advanced to 0.99.15ex07.

Other changes purely cosmetic -- eg changing some 'int' to 'bool',
and a few small documentation edits.

5 files changed, 30 insertions, 36 deletions

diff --git a/bgpd/bgp.h b/bgpd/bgp.h
index 1016fe93..6019b33c 100644
--- a/bgpd/bgp.h
+++ b/bgpd/bgp.h
@@ -84,7 +84,7 @@
  *   RFC4264  BGP Wedgies
  *   RFC4098  Terminology for Benchmarking BGP Device Convergence in the
  *            Control Plane
- *   RFC3822  Configuring BGP to Block Denial-of-Service Attack
+ *   RFC3882  Configuring BGP to Block Denial-of-Service Attack
  *   RFC3765  NOPEER Community
  *   RFC3682 ...see RFC5082
  *   RFC3392 ...see RFC5492                    -- obsoletes RFC2842
diff --git a/bgpd/bgp_connection.c b/bgpd/bgp_connection.c
index 4a8109a3..e0bc88b6 100644
--- a/bgpd/bgp_connection.c
+++ b/bgpd/bgp_connection.c
@@ -658,24 +658,21 @@ bgp_connection_start(bgp_connection connection, union sockunion* su_local,
 } ;
 
 /*------------------------------------------------------------------------------
- * Stop connection
+ * Stop connection buffering -- may leave write buffer to be emptied.
  *
  *   * reset stream buffers
  *   * empty out any pending queue
  *   * remove from the BGP Engine connection queue, if there
- *   * clear session->active flag, so will not process any more messages
- *     that expect some message to be sent.
  *   * no notification pending (yet)
  *
  * If required:
  *
- *   * set write buffer unwritable
- *   * disable file in write mode
+ *   * set write buffer unwritable and empty
  *
  * NB: requires the session to be LOCKED.
  */
 static void
-bgp_connection_stop(bgp_connection connection, int stop_writer)
+bgp_connection_stop(bgp_connection connection, bool stop_writer)
 {
   /* Reset all stream buffering empty.                                  */
   stream_reset(connection->ibuf) ;
@@ -705,7 +702,8 @@ bgp_connection_enable_accept(bgp_connection connection)
   bgp_session session = connection->session ;
 
   assert(connection->ordinal == bgp_connection_secondary) ;
-  assert((session != NULL) && (session->active)) ;
+  assert(session != NULL) ;
+  assert(session->active) ;
 
   session->accept = true ;
 } ;
@@ -821,8 +819,8 @@ bgp_connection_full_close(bgp_connection connection, int unset_timers)
   sockunion_unset(&connection->su_local) ;
   sockunion_unset(&connection->su_remote) ;
 
-  /* Bring connection to a stop.                                        */
-  bgp_connection_stop(connection, 1) ;
+  /* Stop all buffering activity, including write buffer.               */
+  bgp_connection_stop(connection, true) ;
 } ;
 
 /*------------------------------------------------------------------------------
@@ -848,10 +846,9 @@ bgp_connection_full_close(bgp_connection connection, int unset_timers)
  *
  * NB: requires the session to be LOCKED.
  */
-extern int
+extern bool
 bgp_connection_part_close(bgp_connection connection)
 {
-  bgp_session session = connection->session ;
   bgp_wbuffer wb = &connection->wbuff ;
   int         fd ;
   uint8_t*    p ;
@@ -861,18 +858,14 @@ bgp_connection_part_close(bgp_connection connection)
   fd = qps_file_fd(connection->qf) ;
 
   if (fd == fd_undef)
-    return 0 ;
+    return false ;
 
   /* Shutdown the read side of this connection                          */
   shutdown(fd, SHUT_RD) ;
   qps_disable_modes(connection->qf, qps_read_mbit) ;
 
   /* Stop all buffering activity, except for write buffer.              */
-  bgp_connection_stop(connection, 0) ;
-
-  /* Turn off session->active (if still attached).                      */
-  if (session != NULL)
-    session->active = false ;
+  bgp_connection_stop(connection, false) ;
 
   /* Purge wbuff of all but current partly written message (if any)     */
   if (wb->p_in != wb->p_out)    /* will be equal if buffer is empty     */
@@ -898,7 +891,7 @@ bgp_connection_part_close(bgp_connection connection)
     bgp_write_buffer_reset(wb) ;
 
   /* OK -- part closed, ready to send NOTIFICATION              */
-  return 1 ;
+  return true ;
 } ;
 
 /*==============================================================================
@@ -969,7 +962,7 @@ bgp_connection_write_action(qps_file qf, void* file_info)
   int ret ;
 
   /* Try to empty the write buffer.                                     */
-  have = bgp_write_buffer_pending(wb) ;
+  have = bgp_write_buffer_has(wb) ;
   while (have != 0)
     {
       ret = write(qps_file_fd(qf), wb->p_out, have) ;
diff --git a/bgpd/bgp_connection.h b/bgpd/bgp_connection.h
index 9e7d4086..304724cd 100644
--- a/bgpd/bgp_connection.h
+++ b/bgpd/bgp_connection.h
@@ -237,10 +237,10 @@ bgp_connection_make_primary(bgp_connection connection) ;
 extern void
 bgp_connection_full_close(bgp_connection connection, int unset_timers) ;
 
-#define bgp_connection_close(conn) bgp_connection_full_close(conn, 0)
-#define bgp_connection_close_down(conn) bgp_connection_full_close(conn, 1)
+#define bgp_connection_close(conn) bgp_connection_full_close(conn, false)
+#define bgp_connection_close_down(conn) bgp_connection_full_close(conn, true)
 
-extern int
+extern bool
 bgp_connection_part_close(bgp_connection connection) ;
 
 extern void
@@ -261,7 +261,7 @@ bgp_connection_queue_del(bgp_connection connection) ;
 extern int
 bgp_connection_queue_process(void) ;
 
-Inline int
+Inline bool
 bgp_connection_no_pending(bgp_connection connection, bgp_connection* is_pending)
 {
   return (   (mqueue_local_head(&connection->pending_queue) == NULL)
@@ -296,7 +296,7 @@ bgp_write_buffer_reset(bgp_wbuffer wb)
  *
  * NB: there is never any room in an unallocated buffer.
  */
-Inline int
+Inline bool
 bgp_write_buffer_cannot(bgp_wbuffer wb, size_t want)
 {
   return ((size_t)(wb->limit - wb->p_in) <= want) ;
@@ -309,7 +309,7 @@ bgp_write_buffer_cannot(bgp_wbuffer wb, size_t want)
  */
 enum { bgp_write_buffer_full_threshold = BGP_MSG_MAX_L + 1 } ;
 
-Inline int
+Inline bool
 bgp_write_buffer_cannot_max(bgp_wbuffer wb)
 {
   return bgp_write_buffer_cannot(wb, BGP_MSG_MAX_L) ;
@@ -321,11 +321,11 @@ bgp_write_buffer_cannot_max(bgp_wbuffer wb)
  * If empty, ensures that the buffer has been allocated, and sets the pointers
  * to the start of the buffer -- so all set to go.
  */
-Inline int
+Inline bool
 bgp_write_buffer_empty(bgp_wbuffer wb)
 {
   if (wb->p_out < wb->p_in)
-    return 0 ;                  /* not empty => has buffer      */
+    return false ;              /* not empty => has buffer      */
 
   dassert(wb->p_out == wb->p_in) ;
 
@@ -333,7 +333,7 @@ bgp_write_buffer_empty(bgp_wbuffer wb)
 
   bgp_write_buffer_reset(wb) ;  /* pointers to start of buffer  */
 
-  return 1 ;                    /* empty and all ready to go    */
+  return true ;                 /* empty and all ready to go    */
 } ;
 
 /*------------------------------------------------------------------------------
@@ -344,7 +344,7 @@ bgp_write_buffer_empty(bgp_wbuffer wb)
  *     > 0 => allocated.
  */
 Inline int
-bgp_write_buffer_pending(bgp_wbuffer wb)
+bgp_write_buffer_has(bgp_wbuffer wb)
 {
   dassert(wb->p_out <= wb->p_in) ;
   return (wb->p_in - wb->p_out) ;
@@ -353,7 +353,7 @@ bgp_write_buffer_pending(bgp_wbuffer wb)
 /*------------------------------------------------------------------------------
  * As above, for connection
  */
-Inline int
+Inline bool
 bgp_connection_write_cannot_max(bgp_connection connection)
 {
   return bgp_write_buffer_cannot_max(&connection->wbuff) ;
@@ -362,7 +362,7 @@ bgp_connection_write_cannot_max(bgp_connection connection)
 /*------------------------------------------------------------------------------
  * As above, for connection
  */
-Inline int
+Inline bool
 bgp_connection_write_empty(bgp_connection connection)
 {
   return bgp_write_buffer_empty(&connection->wbuff) ;
diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index b1c085bd..3ce8cc19 100644
--- a/bgpd/bgp_fsm.c
+++ b/bgpd/bgp_fsm.c
@@ -2146,7 +2146,11 @@ bgp_fsm_catch(bgp_connection connection, bgp_fsm_state_t next_state)
    *
    * The state transition stuff looks after timers.  In particular an error
    * in Connect/Active states leaves the ConnectRetryTimer running.
+   *
+   * However, in any event, no longer require any Keepalive.
    */
+  qtimer_unset(connection->keepalive_timer) ;
+
   if ((send_notification != NULL) && bgp_connection_part_close(connection))
     {
       /* If not changing to stopping, we hold in the current state until
@@ -2155,9 +2159,6 @@ bgp_fsm_catch(bgp_connection connection, bgp_fsm_state_t next_state)
       if (next_state != bgp_fsm_sStopping)
         next_state = connection->state ;
 
-      /* Make sure that cannot pop out a Keepalive !                        */
-      qtimer_unset(connection->keepalive_timer) ;
-
       /* Write the message                                                  */
       bgp_msg_write_notification(connection, send_notification) ;
 
diff --git a/configure.ac b/configure.ac
index 983ebae9..49410d03 100755
--- a/configure.ac
+++ b/configure.ac
@@ -8,7 +8,7 @@
 ## $Id$
 AC_PREREQ(2.53)
 
-AC_INIT(Quagga, 0.99.15ex06, [http://bugzilla.quagga.net])
+AC_INIT(Quagga, 0.99.15ex07, [http://bugzilla.quagga.net])
 AC_CONFIG_SRCDIR(lib/zebra.h)
 AC_CONFIG_MACRO_DIR([m4])