Trap SIGABRT so that get backtrace in the log.

Improve handling of notification objects, and the printing of same
to the logs.

8 files changed, 138 insertions, 48 deletions

diff --git a/bgpd/bgp_debug.c b/bgpd/bgp_debug.c
index 684e9651..c4cf9cf6 100644
--- a/bgpd/bgp_debug.c
+++ b/bgpd/bgp_debug.c
@@ -262,6 +262,7 @@ bgp_notify_print(struct peer *peer, bgp_notify notification)
   bool  log_neighbor_changes ;
   int   length ;
   char* alloc ;
+  int subcode ;
 
   /* See if we need to do any of this                                   */
   if      (bgp_flag_check (peer->bgp, BGP_FLAG_LOG_NEIGHBOR_CHANGES))
@@ -274,30 +275,37 @@ bgp_notify_print(struct peer *peer, bgp_notify notification)
   /* Sort out string forms of code and subcode                          */
   code_str = LOOKUP (bgp_notify_msg, notification->code) ;
 
-  subcode_str = "";
+  subcode     = notification->subcode ;
+  subcode_str = "/Unspecific";
 
   switch (notification->code)
     {
     case BGP_NOTIFY_HEADER_ERR:
-      subcode_str = LOOKUP (bgp_notify_head_msg, notification->subcode);
+      if (subcode != 0)
+        subcode_str = LOOKUP (bgp_notify_head_msg, subcode);
       break;
     case BGP_NOTIFY_OPEN_ERR:
-      subcode_str = LOOKUP (bgp_notify_open_msg, notification->subcode);
+      if (subcode != 0)
+        subcode_str = LOOKUP (bgp_notify_open_msg, subcode);
       break;
     case BGP_NOTIFY_UPDATE_ERR:
-      subcode_str = LOOKUP (bgp_notify_update_msg, notification->subcode);
+      if (subcode != 0)
+        subcode_str = LOOKUP (bgp_notify_update_msg, subcode);
       break;
     case BGP_NOTIFY_HOLD_ERR:
-      subcode_str = "";
-      break;
     case BGP_NOTIFY_FSM_ERR:
-      subcode_str = "";
+      if (subcode != 0)
+        subcode_str = "/*unknown*" ;
+      else
+        subcode_str = "";
       break;
     case BGP_NOTIFY_CEASE:
-      subcode_str = LOOKUP (bgp_notify_cease_msg, notification->subcode);
+      if (subcode != 0)
+        subcode_str = LOOKUP (bgp_notify_cease_msg, subcode);
       break;
     case BGP_NOTIFY_CAPABILITY_ERR:
-      subcode_str = LOOKUP (bgp_notify_capability_msg, notification->subcode);
+      if (subcode != 0)
+        subcode_str = LOOKUP (bgp_notify_capability_msg, subcode);
       break;
     }
 
@@ -338,7 +346,7 @@ bgp_notify_print(struct peer *peer, bgp_notify notification)
 	       notification->code, notification->subcode,
 	       code_str, subcode_str, length, hex_form) ;
 
-  /* Release the */
+  /* Release the space allocated to the hex form of the data, if any    */
   if (alloc != NULL)
     XFREE(MTYPE_TMP, alloc) ;
 } ;
diff --git a/bgpd/bgp_notification.c b/bgpd/bgp_notification.c
index 45bf99b7..c0690df9 100644
--- a/bgpd/bgp_notification.c
+++ b/bgpd/bgp_notification.c
@@ -203,19 +203,25 @@ bgp_notify_take(bgp_notify* p_notification)
 extern void
 bgp_notify_set(bgp_notify* p_dst, bgp_notify src)
 {
-  bgp_notify_free(*p_dst) ;
-  *p_dst = src ;
+  if (*p_dst != src)            /* empty operation if already set !     */
+    {
+      bgp_notify_free(*p_dst) ;
+      *p_dst = src ;
+    } ;
 } ;
 
 /*------------------------------------------------------------------------------
  * Set pointer to notification to a *copy* of the source.
  *
- * Frees any existing notification at the destination.
+ * Frees any existing notification at the destination unless points at src !
  */
 extern void
 bgp_notify_set_dup(bgp_notify* p_dst, bgp_notify src)
 {
-  bgp_notify_set(p_dst, bgp_notify_dup(src)) ;
+  if (*p_dst != src)
+    bgp_notify_free(*p_dst) ;   /* avoid freeing what we're duplicating */
+
+  *p_dst = bgp_notify_dup(src) ;
 } ;
 
 /*------------------------------------------------------------------------------
@@ -228,8 +234,7 @@ bgp_notify_set_dup(bgp_notify* p_dst, bgp_notify src)
 extern void
 bgp_notify_set_mov(bgp_notify* p_dst, bgp_notify* p_src)
 {
-  bgp_notify_free(*p_dst) ;
-  *p_dst = *p_src ;
+  bgp_notify_set(p_dst, *p_src) ;
   *p_src = NULL ;
 } ;
 
diff --git a/bgpd/bgp_peer.c b/bgpd/bgp_peer.c
index 2062c527..625e1d99 100644
--- a/bgpd/bgp_peer.c
+++ b/bgpd/bgp_peer.c
@@ -177,7 +177,8 @@
 /* prototypes */
 
 static void bgp_session_has_established(bgp_session session);
-static void bgp_session_has_stopped(bgp_session session);
+static void bgp_session_has_stopped(bgp_session session,
+                                                      bgp_notify notification) ;
 static void bgp_session_has_disabled(bgp_session session);
 static void bgp_uptime_reset (struct peer *peer);
 static void bgp_peer_stop (struct peer *peer, bool nsf) ;
@@ -216,9 +217,7 @@ bgp_session_do_event(mqueue_block mqb, mqb_flag_t flag)
     {
       /* Pull stuff into Routing Engine *private* fields in the session     */
 
-      session->event = args->event ;      /* last event                     */
-      bgp_notify_set(&session->notification, args->notification) ;
-                                          /* if any sent/received           */
+      session->event   = args->event ;    /* last event                     */
       session->err     = args->err ;      /* errno, if any                  */
       session->ordinal = args->ordinal ;  /* primary/secondary connection   */
 
@@ -232,6 +231,8 @@ bgp_session_do_event(mqueue_block mqb, mqb_flag_t flag)
          * before the BGP Engine had seen the disable message.
          */
         case bgp_session_eEstablished:
+          assert(args->notification == NULL) ;
+
           if (session->state == bgp_session_sLimping)
             break ;
 
@@ -241,6 +242,10 @@ bgp_session_do_event(mqueue_block mqb, mqb_flag_t flag)
         /* If now Disabled, then the BGP Engine is acknowledging the a
          * session disable, and the session is now disabled.
          *
+         * If sent a notification with the disable request, then it is
+         * returned iff the notification was actually sent.  Don't really
+         * care one way or the other.
+         *
          * BEWARE: this may be the last thing that happens to the session
          *         and/or the related peer -- which may be deleted inside
          *         bgp_session_has_disabled().
@@ -259,12 +264,13 @@ bgp_session_do_event(mqueue_block mqb, mqb_flag_t flag)
             break ;
 
           if (args->stopped)
-            bgp_session_has_stopped(session) ;
+            bgp_session_has_stopped(session,
+                                       bgp_notify_take(&(args->notification))) ;
           break ;
       } ;
-    }
-  else
-    bgp_notify_free(args->notification) ;
+    } ;
+
+  bgp_notify_free(args->notification) ;  /* Discard any notification.    */
 
   mqb_free(mqb) ;
 }
@@ -410,11 +416,13 @@ bgp_session_has_established(bgp_session session)
  * that is to tell it to disable the session, and then wait in sLimping state
  * until the BGP Engine completes the disable request and signals that.
  *
+ * NB: takes responsibility for the notification.
+ *
  * TODO: session stopped because we stopped it or because the other end did ?
  * TODO: restore NSF !!
  */
 static void
-bgp_session_has_stopped(bgp_session session)
+bgp_session_has_stopped(bgp_session session, bgp_notify notification)
 {
   peer_down_t why_down ;
 
@@ -432,17 +440,17 @@ bgp_session_has_stopped(bgp_session session)
         peer->v_start = (60 * 2);
     } ;
 
-  if (session->notification == NULL)
+  if (notification == NULL)
     why_down = PEER_DOWN_CLOSE_SESSION ;
   else
     {
-      if (session->notification->received)
+      if (notification->received)
         why_down = PEER_DOWN_NOTIFY_RECEIVED ;
       else
-        why_down = bgp_peer_map_notification(session->notification) ;
+        why_down = bgp_peer_map_notification(notification) ;
     } ;
 
-  bgp_peer_down_notify(peer, why_down, session->notification) ;
+  bgp_peer_down_notify(peer, why_down, notification) ;
 } ;
 
 /*------------------------------------------------------------------------------
@@ -1255,6 +1263,16 @@ bgp_peer_down(bgp_peer peer, peer_down_t why_down)
   bgp_peer_down_notify(peer, why_down, NULL) ;
 } ;
 
+/*------------------------------------------------------------------------------
+ * Down Peer for the given reason, with the given notification, if any.
+ *
+ * See bgp_peer_down() above.
+ *
+ * If the notification is NULL and need to send a notification, make one up from
+ * the given reason for downing the peer.
+ *
+ * NB: takes responsibility for the notification.
+ */
 static void
 bgp_peer_down_notify(bgp_peer peer, peer_down_t why_down,
                                                         bgp_notify notification)
@@ -1266,12 +1284,14 @@ bgp_peer_down_notify(bgp_peer peer, peer_down_t why_down,
       if (notification == NULL)
         notification = bgp_peer_map_peer_down(why_down) ;
 
-      bgp_notify_set_dup(&peer->session->notification, notification) ;
+      bgp_notify_set(&peer->session->notification, notification) ;
 
-      bgp_session_disable(peer, notification) ;
-    }
-  else
-    bgp_notify_free(notification) ;     /* Discard unused notification  */
+      bgp_session_disable(peer, bgp_notify_dup(notification)) ;
+              /* The copy of the notification will be discarded either by
+               * the BGP_Engine, if the notification is not sent, or when
+               * it is returned in the eDisabled message.
+               */
+    } ;
 
   /* Now worry about the state of the peer                              */
 
diff --git a/lib/log.c b/lib/log.c
index 32b11b76..72f967f6 100644
--- a/lib/log.c
+++ b/lib/log.c
@@ -38,6 +38,7 @@
 #endif
 #include "qpthreads.h"
 #include "qfstring.h"
+#include "sigevent.h"
 
 /* log is protected by the same mutext as vty, see comments in vty.c */
 
@@ -519,6 +520,13 @@ zlog_signal(int signo, const char *action
 #undef LOC
 }
 
+/* Ring down the curtain -- turn of SIGABRT handler and abort()         */
+void zabort_abort(void)
+{
+  quagga_sigabrt_no_trap() ;
+  abort() ;
+}
+
 /* Log a backtrace using only async-signal-safe functions.
    Needs to be enhanced to support syslog logging. */
 void
@@ -776,7 +784,7 @@ zlog_abort (const char *mess)
 
   uzlog(NULL, LOG_CRIT, "%s", mess);
   uzlog_backtrace(LOG_CRIT);
-  abort();
+  zabort_abort();
 }
 
 
diff --git a/lib/log.h b/lib/log.h
index 1894aff2..dda773ea 100644
--- a/lib/log.h
+++ b/lib/log.h
@@ -193,6 +193,9 @@ extern void zlog_signal(int signo, const char *action
 #endif
 		       );
 
+/* Ring down the curtain -- turn of SIGABRT handler and abort()         */
+extern void zabort_abort(void)  __attribute__ ((noreturn)) ;
+
 /* Log a backtrace. */
 extern void zlog_backtrace(int priority);
 
diff --git a/lib/memory.c b/lib/memory.c
index 5ee33aa8..2e295566 100644
--- a/lib/memory.c
+++ b/lib/memory.c
@@ -107,7 +107,7 @@ zerror (const char *fname, int type, size_t size)
      unfortunately zlog_backtrace_sigsafe does not support syslog logging at
      this time... */
   zlog_backtrace(LOG_WARNING);
-  abort();
+  zabort_abort();
 }
 
 /*------------------------------------------------------------------------------
@@ -205,13 +205,15 @@ zfree (enum MTYPE mtype, void *ptr)
 {
   LOCK ;
 
-  free (ptr);
+  assert(mstat.mt[mtype].alloc > 0) ;
 
   mstat.mt[mtype].alloc--;
 #ifdef MEMORY_TRACKER
   mem_md_free(mtype, ptr) ;
 #endif
 
+  free (ptr);
+
   UNLOCK ;
 } ;
 
diff --git a/lib/sigevent.c b/lib/sigevent.c
index 6e50d631..18fcffb0 100644
--- a/lib/sigevent.c
+++ b/lib/sigevent.c
@@ -220,18 +220,25 @@ core_handler(int signo
 	      , siginfo, program_counter(context)
 #endif
 	     );
-  abort();
+  zabort_abort();
 }
 
+/* For the signals known to Quagga, and which are in their default state,
+ * set a Quagga default handler.
+ */
 static void
 trap_default_signals(void)
 {
   static const int core_signals[] = {
     SIGQUIT,
     SIGILL,
+    SIGABRT,
 #ifdef SIGEMT
     SIGEMT,
 #endif
+#ifdef SIGIOT
+    SIGIOT,
+#endif
     SIGFPE,
     SIGBUS,
     SIGSEGV,
@@ -245,6 +252,7 @@ trap_default_signals(void)
     SIGXFSZ,
 #endif
   };
+
   static const int exit_signals[] = {
     SIGHUP,
     SIGINT,
@@ -262,9 +270,11 @@ trap_default_signals(void)
     SIGSTKFLT,
 #endif
   };
+
   static const int ignore_signals[] = {
     SIGPIPE,
   };
+
   static const struct {
     const int *sigs;
     u_int nsigs;
@@ -287,28 +297,39 @@ trap_default_signals(void)
       for (j = 0; j < sigmap[i].nsigs; j++)
         {
 	  struct sigaction oact;
-	  if ((sigaction(sigmap[i].sigs[j],NULL,&oact) == 0) &&
-	      (oact.sa_handler == SIG_DFL))
+          if (sigaction(sigmap[i].sigs[j], NULL, &oact) < 0)
+            zlog_warn("Unable to get signal handler for signal %d: %s",
+                      sigmap[i].sigs[j], errtoa(errno, 0).str);
+          else {
+#ifdef SA_SIGINFO
+            if (oact.sa_flags & SA_SIGINFO)
+              continue ;                /* Don't set again              */
+#endif
+            if (oact.sa_handler != SIG_DFL)
+              continue ;                /* Don't set again              */
+          }
+	  if ( (sigaction(sigmap[i].sigs[j], NULL, &oact) == 0) &&
+	                                          (oact.sa_handler == SIG_DFL) )
 	    {
 	      struct sigaction act;
 	      sigfillset (&act.sa_mask);
 	      if (sigmap[i].handler == NULL)
 	        {
 		  act.sa_handler = SIG_IGN;
-		  act.sa_flags = 0;
+		  act.sa_flags   = 0;
 	        }
 	      else
 	        {
 #ifdef SA_SIGINFO
 		  /* Request extra arguments to signal handler. */
 		  act.sa_sigaction = sigmap[i].handler;
-		  act.sa_flags = SA_SIGINFO;
+		  act.sa_flags     = SA_SIGINFO;
 #else
-		  act.sa_handler = sigmap[i].handler;
-		  act.sa_flags = 0;
+		  act.sa_handler   = sigmap[i].handler;
+		  act.sa_flags     = 0;
 #endif
 	        }
-	      if (sigaction(sigmap[i].sigs[j],&act,NULL) < 0)
+	      if (sigaction(sigmap[i].sigs[j], &act, NULL) < 0)
 	        zlog_warn("Unable to set signal handler for signal %d: %s",
 			  sigmap[i].sigs[j], errtoa(errno, 0).str);
 
@@ -346,3 +367,23 @@ signal_init (struct thread_master *m, int sigc,
                       QUAGGA_SIGNAL_TIMER_INTERVAL);
 #endif /* SIGEVENT_SCHEDULE_THREAD */
 }
+
+/* turn off trap for SIGABRT !                                  */
+extern void quagga_sigabrt_no_trap(void)
+{
+  struct sigaction new_act ;
+  sigset_t set ;
+
+  sigfillset(&set) ;
+
+  new_act.sa_handler = SIG_DFL ;
+  new_act.sa_mask    = set ;
+  new_act.sa_flags   = 0 ;
+  sigaction(SIGABRT, &new_act, NULL) ;
+
+  sigemptyset(&set) ;
+  sigaddset(&set, SIGABRT) ;
+  sigprocmask(SIG_UNBLOCK, &set, NULL) ;
+
+} ;
+
diff --git a/lib/sigevent.h b/lib/sigevent.h
index 62b944a7..57486bc2 100644
--- a/lib/sigevent.h
+++ b/lib/sigevent.h
@@ -1,4 +1,4 @@
-/* 
+/*
  * Quagga Signal handling header.
  *
  * Copyright (C) 2004 Paul Jakma.
@@ -18,7 +18,7 @@
  * You should have received a copy of the GNU General Public License
  * along with Quagga; see the file COPYING.  If not, write to the Free
  * Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
- * 02111-1307, USA.  
+ * 02111-1307, USA.
  */
 
 #ifndef _QUAGGA_SIGNAL_H
@@ -44,10 +44,13 @@ struct quagga_signal_t
  * - array of quagga_signal_t's describing signals to handle
  *   and handlers to use for each signal
  */
-extern void signal_init (struct thread_master *m, int sigc, 
+extern void signal_init (struct thread_master *m, int sigc,
                          struct quagga_signal_t *signals);
 
 /* check whether there are signals to handle, process any found */
 extern int quagga_sigevent_process (void);
 
+/* turn off trap for SIGABRT !                                  */
+extern void quagga_sigabrt_no_trap(void) ;
+
 #endif /* _QUAGGA_SIGNAL_H */