SNMP: Better handling of errors

proto/snmp/snmp.c

@@ -54,6 +54,15 @@
  *    +-----------------+
  *
  *
+ *    +-----------------+
+ *    | SNMP_RESET      |     waiting to transmit response to malformed packet
+ *    +-----------------+
+ *	 |
+ *       |    response was send, reseting the session (with socket)
+ *       |
+ *	 \--> SNMP_LOCKED
+ *
+ *
  *  Erroneous transitions:
  *    SNMP is UP in states SNMP_CONN and also in SNMP_REGISTER because the
  *    session is establised and the GetNext request should be responsed
@@ -196,15 +205,15 @@ snmp_reconnect(timer *tm)
   snmp_connected(p->sock);
 }
 
-static void
-snmp_sock_err(sock *sk, int UNUSED err)
+/* this function is internal and shouldn't be used outside the snmp module */
+void
+snmp_sock_disconnect(struct snmp_proto *p, int reconnect)
 {
-  snmp_log("socket error '%s' (errno: %d)", strerror(err), err);
-  struct snmp_proto *p = sk->data;
-  p->errs++;
-
   tm_stop(p->ping_timer);
 
+  if (!reconnect)
+    return snmp_down(p);
+
   proto_notify_state(&p->p, PS_START);
   rfree(p->sock);
   p->sock = NULL;
@@ -212,10 +221,20 @@ snmp_sock_err(sock *sk, int UNUSED err)
   snmp_log("changing state to LOCKED");
   p->state = SNMP_LOCKED;
 
+  /* We try to reconnect after a short delay */
   p->startup_timer->hook = snmp_startup_timeout;
-  tm_start(p->startup_timer,  4 S); // TODO make me configurable
+  tm_start(p->startup_timer, 4 S);  // TODO make me configurable
 }
 
+static void
+snmp_sock_err(sock *sk, int UNUSED err)
+{
+  snmp_log("socket error '%s' (errno: %d)", strerror(err), err);
+  struct snmp_proto *p = sk->data;
+  p->errs++;
+
+  snmp_sock_disconnect(p, 1);
+}
 
 static void
 snmp_start_locked(struct object_lock *lock)

proto/snmp/snmp.h

@@ -37,6 +37,7 @@ enum snmp_proto_state {
   SNMP_CONN,
   SNMP_STOP,
   SNMP_DOWN,
+  SNMP_RESET,
 };
 
 /* hash table macros */
@@ -169,6 +170,7 @@ void snmp_startup(struct snmp_proto *p);
 void snmp_connected(sock *sk);
 void snmp_startup_timeout(timer *tm);
 void snmp_reconnect(timer *tm);
+void snmp_sock_disconnect(struct snmp_proto *p, int reconnect);
 
 
 #endif

proto/snmp/subagent.c

@@ -35,6 +35,7 @@ static struct agentx_response *prepare_response(struct snmp_proto *p, struct snm
 static void response_err_ind(struct agentx_response *res, uint err, uint ind);
 static uint update_packet_size(struct snmp_proto *p, const byte *start, byte *end);
 static struct oid *search_mib(struct snmp_proto *p, const struct oid *o_start, const struct oid *o_end, struct oid *o_curr, struct snmp_pdu *c, enum snmp_search_res *result);
+static void snmp_tx(sock *sk);
 
 u32 snmp_internet[] = { SNMP_ISO, SNMP_ORG, SNMP_DOD, SNMP_INTERNET };
 
@@ -76,6 +77,22 @@ static const char * const snmp_pkt_type[] UNUSED = {
   [AGENTX_RESPONSE_PDU]		  =  "Response-PDU",
 };
 
+static int
+snmp_rx_skip(sock UNUSED *sk, uint UNUSED size)
+{
+  return 1;
+}
+
+static inline void
+snmp_error(struct snmp_proto *p)
+{
+  snmp_log("changing state to RESET");
+  p->state = SNMP_RESET;
+
+  p->sock->rx_hook = snmp_rx_skip;
+  p->sock->tx_hook = snmp_tx;
+}
+
 static void
 snmp_simple_response(struct snmp_proto *p, enum agentx_response_err error, u16 index)
 {
@@ -313,27 +330,28 @@ close_pdu(struct snmp_proto *p, u8 reason)
 #undef REASON_SIZE
 }
 
-static uint UNUSED
-parse_close_pdu(struct snmp_proto UNUSED *p, byte UNUSED *req, uint UNUSED size)
+static uint
+parse_close_pdu(struct snmp_proto *p, byte * const pkt_start, uint size)
 {
-#if 0
-  //byte *pkt = req;
-  //sock *sk = p->sock;
+  byte *pkt = pkt_start;
+  struct agentx_header *h = (void *) pkt;
+  ADVANCE(pkt, size, AGENTX_HEADER_SIZE);
+  int byte_ord = h->flags & AGENTX_NETWORK_BYTE_ORDER;
+  uint pkt_size = LOAD_U32(h->payload, byte_ord);
 
-  if (size < sizeof(struct agentx_header))
+  if (pkt_size != 0)
   {
-    return 0;
+    snmp_simple_response(p, AGENTX_RES_GEN_ERROR, 0);
+    // TODO: best solution for possibly malicious pkt_size
+    //return AGENTX_HEADER_SIZE + MIN(size, pkt_size);
+    return AGENTX_HEADER_SIZE;
   }
 
-  //struct agentx_header *h = (void *) req;
-  ADVANCE(req, size, AGENTX_HEADER_SIZE);
+  /* The agentx-Close-PDU must not have non-default context */
 
-  p->state = SNMP_ERR;
-
-  /* or snmp_cleanup(); // ??! */
-  proto_notify_state(&p->p, PS_DOWN);
-#endif
-  return 0;
+  snmp_simple_response(p, AGENTX_RES_NO_ERROR, 0);
+  snmp_sock_disconnect(p, 1); // TODO: should we try to reconnect (2nd arg) ??
+  return AGENTX_HEADER_SIZE;
 }
 
 /* MUCH better signature would be
@@ -541,7 +559,10 @@ error:
   s = update_packet_size(p, sk->tpos, c.buffer);
 
   if (c.error != AGENTX_RES_NO_ERROR)
+  {
     response_err_ind(res, c.error, c.index + 1);
+    snmp_error(p);
+  }
   else if (all_possible)
     response_err_ind(res, AGENTX_RES_NO_ERROR, 0);
   else
@@ -604,6 +625,11 @@ parse_set_pdu(struct snmp_proto *p, byte * const pkt_start, uint size, uint err)
 error:;
   response_err_ind(r, c.error, 0);
   sk_send(p->sock, AGENTX_HEADER_SIZE);
+
+  /* Reset the connection on unrecoverable error */
+  if (c.error != AGENTX_RES_NO_ERROR && c.error != err)
+    snmp_error(p);
+
   return pkt - pkt_start;
 }
 
@@ -1374,6 +1400,16 @@ snmp_rx(sock *sk, uint size)
   return 1;
 }
 
+/* snmp_tx - used to reset the connection when the
+ *   agentx-Response-PDU was sent
+ */
+static void
+snmp_tx(sock *sk)
+{
+  struct snmp_proto *p = sk->data;
+  snmp_sock_disconnect(p, 1);
+}
+
 /* Ping-PDU */
 void
 snmp_ping(struct snmp_proto *p)