Session/tcp coverity fixes

Change-Id: Ic5467df16e870b49c49678b1dbb40f4a2390b3c9
Signed-off-by: Florin Coras <fcoras@cisco.com>

7 files changed, 96 insertions, 102 deletions

diff --git a/src/vnet/buffer.h b/src/vnet/buffer.h
index ed869d1f768..5d1b1c4d699 100644
--- a/src/vnet/buffer.h
+++ b/src/vnet/buffer.h
@@ -83,7 +83,8 @@ _(policer)                                      \
 _(ipsec)					\
 _(map)						\
 _(map_t)					\
-_(ip_frag)
+_(ip_frag)					\
+_(tcp)
 
 /*
  * vnet stack buffer opaque array overlay structure.
@@ -279,6 +280,9 @@ typedef struct
       u32 seq_number;
       u32 seq_end;
       u32 ack_number;
+      u16 hdr_offset;		/**< offset relative to ip hdr */
+      u16 data_offset;		/**< offset relative to ip hdr */
+      u16 data_len;		/**< data len */
       u8 flags;
     } tcp;
 
diff --git a/src/vnet/session/session_api.c b/src/vnet/session/session_api.c
index 79d67a2fece..5a02a08e003 100755
--- a/src/vnet/session/session_api.c
+++ b/src/vnet/session/session_api.c
@@ -227,6 +227,12 @@ redirect_connect_callback (u32 server_api_client_index, void *mp_arg)
   /* Tell the server the client's API queue address, so it can reply */
   mp->client_queue_address = (u64) client_q;
   app = application_lookup (mp->client_index);
+  if (!app)
+    {
+      clib_warning ("no client application");
+      return -1;
+    }
+
   mp->options[SESSION_OPTIONS_RX_FIFO_SIZE] = app->sm_properties.rx_fifo_size;
   mp->options[SESSION_OPTIONS_TX_FIFO_SIZE] = app->sm_properties.tx_fifo_size;
 
diff --git a/src/vnet/tcp/builtin_client.c b/src/vnet/tcp/builtin_client.c
index 276beb216d2..32d69a9633e 100644
--- a/src/vnet/tcp/builtin_client.c
+++ b/src/vnet/tcp/builtin_client.c
@@ -56,6 +56,9 @@ send_test_chunk (tclient_main_t * tm, session_t * s)
   session_fifo_event_t evt;
   static int serial_number = 0;
   int rv;
+
+  ASSERT (vec_len (test_data) > 0);
+
   test_buf_offset = s->bytes_sent % vec_len (test_data);
   bytes_this_chunk = vec_len (test_data) - test_buf_offset;
 
diff --git a/src/vnet/tcp/tcp.h b/src/vnet/tcp/tcp.h
index 40fb3515635..f61a1b52882 100644
--- a/src/vnet/tcp/tcp.h
+++ b/src/vnet/tcp/tcp.h
@@ -351,6 +351,14 @@ vnet_get_tcp_main ()
   return &tcp_main;
 }
 
+always_inline tcp_header_t *
+tcp_buffer_hdr (vlib_buffer_t * b)
+{
+  ASSERT ((signed) b->current_data >= (signed) -VLIB_BUFFER_PRE_DATA_SIZE);
+  return (tcp_header_t *) (b->data + b->current_data
+			   + vnet_buffer (b)->tcp.hdr_offset);
+}
+
 clib_error_t *vnet_tcp_enable_disable (vlib_main_t * vm, u8 is_en);
 
 always_inline tcp_connection_t *
diff --git a/src/vnet/tcp/tcp_error.def b/src/vnet/tcp/tcp_error.def
index b91a08c0679..0d75d9751d7 100644
--- a/src/vnet/tcp/tcp_error.def
+++ b/src/vnet/tcp/tcp_error.def
@@ -13,6 +13,7 @@
  * limitations under the License.
  */
 tcp_error (NONE, "no error")
+tcp_error (LENGTH, "inconsistent ip/tcp lengths")
 tcp_error (NO_LISTENER, "no listener for dst port")
 tcp_error (LOOKUP_DROPS, "lookup drops")
 tcp_error (DISPATCH, "Dispatch error")
diff --git a/src/vnet/tcp/tcp_input.c b/src/vnet/tcp/tcp_input.c
index e184a4d69e3..3c65a5ea73d 100644
--- a/src/vnet/tcp/tcp_input.c
+++ b/src/vnet/tcp/tcp_input.c
@@ -1176,6 +1176,21 @@ format_tcp_rx_trace_short (u8 * s, va_list * args)
   return s;
 }
 
+void
+tcp_set_rx_trace_data (tcp_rx_trace_t * t0, tcp_connection_t * tc0,
+		       tcp_header_t * th0, vlib_buffer_t * b0, u8 is_ip4)
+{
+  if (tc0)
+    {
+      clib_memcpy (&t0->tcp_connection, tc0, sizeof (t0->tcp_connection));
+    }
+  else
+    {
+      th0 = tcp_buffer_hdr (b0);
+    }
+  clib_memcpy (&t0->tcp_header, th0, sizeof (t0->tcp_header));
+}
+
 always_inline void
 tcp_established_inc_counter (vlib_main_t * vm, u8 is_ip4, u8 evt, u8 val)
 {
@@ -1212,12 +1227,8 @@ tcp46_established_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	{
 	  u32 bi0;
 	  vlib_buffer_t *b0;
-	  tcp_rx_trace_t *t0;
 	  tcp_header_t *th0 = 0;
 	  tcp_connection_t *tc0;
-	  ip4_header_t *ip40;
-	  ip6_header_t *ip60;
-	  u32 n_advance_bytes0, n_data_bytes0;
 	  u32 next0 = TCP_ESTABLISHED_NEXT_DROP, error0 = TCP_ERROR_ENQUEUED;
 
 	  bi0 = from[0];
@@ -1237,32 +1248,13 @@ tcp46_established_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	      goto done;
 	    }
 
-	  /* Checksum computed by ipx_local no need to compute again */
-
-	  if (is_ip4)
-	    {
-	      ip40 = vlib_buffer_get_current (b0);
-	      th0 = ip4_next_header (ip40);
-	      n_advance_bytes0 = (ip4_header_bytes (ip40)
-				  + tcp_header_bytes (th0));
-	      n_data_bytes0 = clib_net_to_host_u16 (ip40->length)
-		- n_advance_bytes0;
-	    }
-	  else
-	    {
-	      ip60 = vlib_buffer_get_current (b0);
-	      th0 = ip6_next_header (ip60);
-	      n_advance_bytes0 = tcp_header_bytes (th0);
-	      n_data_bytes0 = clib_net_to_host_u16 (ip60->payload_length)
-		- n_advance_bytes0;
-	      n_advance_bytes0 += sizeof (ip60[0]);
-	    }
+	  th0 = tcp_buffer_hdr (b0);
 
 	  is_fin = (th0->flags & TCP_FLAG_FIN) != 0;
 
 	  /* SYNs, FINs and data consume sequence numbers */
 	  vnet_buffer (b0)->tcp.seq_end = vnet_buffer (b0)->tcp.seq_number
-	    + tcp_is_syn (th0) + is_fin + n_data_bytes0;
+	    + tcp_is_syn (th0) + is_fin + vnet_buffer (b0)->tcp.data_len;
 
 	  /* TODO header prediction fast path */
 
@@ -1286,8 +1278,9 @@ tcp46_established_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 
 	  /* 7: process the segment text */
 
-	  vlib_buffer_advance (b0, n_advance_bytes0);
-	  error0 = tcp_segment_rcv (tm, tc0, b0, n_data_bytes0, &next0);
+	  vlib_buffer_advance (b0, vnet_buffer (b0)->tcp.data_offset);
+	  error0 = tcp_segment_rcv (tm, tc0, b0,
+				    vnet_buffer (b0)->tcp.data_len, &next0);
 
 	  /* N.B. buffer is rewritten if segment is ooo. Thus, th0 becomes a
 	   * dangling reference. */
@@ -1308,10 +1301,9 @@ tcp46_established_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	  b0->error = node->errors[error0];
 	  if (PREDICT_FALSE (b0->flags & VLIB_BUFFER_IS_TRACED))
 	    {
-	      t0 = vlib_add_trace (vm, node, b0, sizeof (*t0));
-	      clib_memcpy (&t0->tcp_header, th0, sizeof (t0->tcp_header));
-	      clib_memcpy (&t0->tcp_connection, tc0,
-			   sizeof (t0->tcp_connection));
+	      tcp_rx_trace_t *t0 =
+		vlib_add_trace (vm, node, b0, sizeof (*t0));
+	      tcp_set_rx_trace_data (t0, tc0, th0, b0, is_ip4);
 	    }
 
 	  vlib_validate_buffer_enqueue_x1 (vm, node, next_index, to_next,
@@ -1416,9 +1408,6 @@ tcp46_syn_sent_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	  tcp_rx_trace_t *t0;
 	  tcp_header_t *tcp0 = 0;
 	  tcp_connection_t *tc0;
-	  ip4_header_t *ip40;
-	  ip6_header_t *ip60;
-	  u32 n_advance_bytes0, n_data_bytes0;
 	  tcp_connection_t *new_tc0;
 	  u32 next0 = TCP_SYN_SENT_NEXT_DROP, error0 = TCP_ERROR_ENQUEUED;
 
@@ -1436,27 +1425,7 @@ tcp46_syn_sent_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 
 	  ack0 = vnet_buffer (b0)->tcp.ack_number;
 	  seq0 = vnet_buffer (b0)->tcp.seq_number;
-
-	  /* Checksum computed by ipx_local no need to compute again */
-
-	  if (is_ip4)
-	    {
-	      ip40 = vlib_buffer_get_current (b0);
-	      tcp0 = ip4_next_header (ip40);
-	      n_advance_bytes0 = (ip4_header_bytes (ip40)
-				  + tcp_header_bytes (tcp0));
-	      n_data_bytes0 = clib_net_to_host_u16 (ip40->length)
-		- n_advance_bytes0;
-	    }
-	  else
-	    {
-	      ip60 = vlib_buffer_get_current (b0);
-	      tcp0 = ip6_next_header (ip60);
-	      n_advance_bytes0 = tcp_header_bytes (tcp0);
-	      n_data_bytes0 = clib_net_to_host_u16 (ip60->payload_length)
-		- n_advance_bytes0;
-	      n_advance_bytes0 += sizeof (ip60[0]);
-	    }
+	  tcp0 = tcp_buffer_hdr (b0);
 
 	  if (PREDICT_FALSE
 	      (!tcp_ack (tcp0) && !tcp_rst (tcp0) && !tcp_syn (tcp0)))
@@ -1464,7 +1433,7 @@ tcp46_syn_sent_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 
 	  /* SYNs, FINs and data consume sequence numbers */
 	  vnet_buffer (b0)->tcp.seq_end = seq0 + tcp_is_syn (tcp0)
-	    + tcp_is_fin (tcp0) + n_data_bytes0;
+	    + tcp_is_fin (tcp0) + vnet_buffer (b0)->tcp.data_len;
 
 	  /*
 	   *  1. check the ACK bit
@@ -1591,10 +1560,12 @@ tcp46_syn_sent_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	    }
 
 	  /* Read data, if any */
-	  if (n_data_bytes0)
+	  if (vnet_buffer (b0)->tcp.data_len)
 	    {
-	      error0 =
-		tcp_segment_rcv (tm, new_tc0, b0, n_data_bytes0, &next0);
+	      vlib_buffer_advance (b0, vnet_buffer (b0)->tcp.data_offset);
+	      error0 = tcp_segment_rcv (tm, new_tc0, b0,
+					vnet_buffer (b0)->tcp.data_len,
+					&next0);
 	      if (error0 == TCP_ERROR_PURE_ACK)
 		error0 = TCP_ERROR_SYN_ACKS_RCVD;
 	    }
@@ -1720,12 +1691,8 @@ tcp46_rcv_process_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	{
 	  u32 bi0;
 	  vlib_buffer_t *b0;
-	  tcp_rx_trace_t *t0;
 	  tcp_header_t *tcp0 = 0;
 	  tcp_connection_t *tc0;
-	  ip4_header_t *ip40;
-	  ip6_header_t *ip60;
-	  u32 n_advance_bytes0, n_data_bytes0;
 	  u32 next0 = TCP_RCV_PROCESS_NEXT_DROP, error0 = TCP_ERROR_ENQUEUED;
 
 	  bi0 = from[0];
@@ -1744,30 +1711,12 @@ tcp46_rcv_process_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	      goto drop;
 	    }
 
-	  /* Checksum computed by ipx_local no need to compute again */
-
-	  if (is_ip4)
-	    {
-	      ip40 = vlib_buffer_get_current (b0);
-	      tcp0 = ip4_next_header (ip40);
-	      n_advance_bytes0 = (ip4_header_bytes (ip40)
-				  + tcp_header_bytes (tcp0));
-	      n_data_bytes0 = clib_net_to_host_u16 (ip40->length)
-		- n_advance_bytes0;
-	    }
-	  else
-	    {
-	      ip60 = vlib_buffer_get_current (b0);
-	      tcp0 = ip6_next_header (ip60);
-	      n_advance_bytes0 = tcp_header_bytes (tcp0);
-	      n_data_bytes0 = clib_net_to_host_u16 (ip60->payload_length)
-		- n_advance_bytes0;
-	      n_advance_bytes0 += sizeof (ip60[0]);
-	    }
+	  tcp0 = tcp_buffer_hdr (b0);
 
 	  /* SYNs, FINs and data consume sequence numbers */
 	  vnet_buffer (b0)->tcp.seq_end = vnet_buffer (b0)->tcp.seq_number
-	    + tcp_is_syn (tcp0) + tcp_is_fin (tcp0) + n_data_bytes0;
+	    + tcp_is_syn (tcp0) + tcp_is_fin (tcp0)
+	    + vnet_buffer (b0)->tcp.data_len;
 
 	  /*
 	   * Special treatment for CLOSED
@@ -1911,8 +1860,10 @@ tcp46_rcv_process_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	    case TCP_STATE_ESTABLISHED:
 	    case TCP_STATE_FIN_WAIT_1:
 	    case TCP_STATE_FIN_WAIT_2:
-	      vlib_buffer_advance (b0, n_advance_bytes0);
-	      error0 = tcp_segment_rcv (tm, tc0, b0, n_data_bytes0, &next0);
+	      vlib_buffer_advance (b0, vnet_buffer (b0)->tcp.data_offset);
+	      error0 = tcp_segment_rcv (tm, tc0, b0,
+					vnet_buffer (b0)->tcp.data_len,
+					&next0);
 	      break;
 	    case TCP_STATE_CLOSE_WAIT:
 	    case TCP_STATE_CLOSING:
@@ -1964,15 +1915,14 @@ tcp46_rcv_process_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	    }
 	  TCP_EVT_DBG (TCP_EVT_FIN_RCVD, tc0);
 
+	drop:
 	  b0->error = error0 ? node->errors[error0] : 0;
 
-	drop:
 	  if (PREDICT_FALSE (b0->flags & VLIB_BUFFER_IS_TRACED))
 	    {
-	      t0 = vlib_add_trace (vm, node, b0, sizeof (*t0));
-	      clib_memcpy (&t0->tcp_header, tcp0, sizeof (t0->tcp_header));
-	      clib_memcpy (&t0->tcp_connection, tc0,
-			   sizeof (t0->tcp_connection));
+	      tcp_rx_trace_t *t0 =
+		vlib_add_trace (vm, node, b0, sizeof (*t0));
+	      tcp_set_rx_trace_data (t0, tc0, tcp0, b0, is_ip4);
 	    }
 
 	  vlib_validate_buffer_enqueue_x1 (vm, node, next_index, to_next,
@@ -2320,9 +2270,9 @@ tcp46_input_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 
       while (n_left_from > 0 && n_left_to_next > 0)
 	{
+	  int n_advance_bytes0, n_data_bytes0;
 	  u32 bi0;
 	  vlib_buffer_t *b0;
-	  tcp_rx_trace_t *t0;
 	  tcp_header_t *tcp0 = 0;
 	  tcp_connection_t *tc0;
 	  ip4_header_t *ip40;
@@ -2340,10 +2290,16 @@ tcp46_input_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	  b0 = vlib_get_buffer (vm, bi0);
 	  vnet_buffer (b0)->tcp.flags = 0;
 
+	  /* Checksum computed by ipx_local no need to compute again */
+
 	  if (is_ip4)
 	    {
 	      ip40 = vlib_buffer_get_current (b0);
 	      tcp0 = ip4_next_header (ip40);
+	      n_advance_bytes0 = (ip4_header_bytes (ip40)
+				  + tcp_header_bytes (tcp0));
+	      n_data_bytes0 = clib_net_to_host_u16 (ip40->length)
+		- n_advance_bytes0;
 
 	      /* lookup session */
 	      tc0 =
@@ -2359,6 +2315,11 @@ tcp46_input_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	    {
 	      ip60 = vlib_buffer_get_current (b0);
 	      tcp0 = ip6_next_header (ip60);
+	      n_advance_bytes0 = tcp_header_bytes (tcp0);
+	      n_data_bytes0 = clib_net_to_host_u16 (ip60->payload_length)
+		- n_advance_bytes0;
+	      n_advance_bytes0 += sizeof (ip60[0]);
+
 	      tc0 =
 		(tcp_connection_t *)
 		stream_session_lookup_transport6 (&ip60->src_address,
@@ -2369,6 +2330,13 @@ tcp46_input_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 						  my_thread_index);
 	    }
 
+	  /* Length check */
+	  if (PREDICT_FALSE (n_advance_bytes0 < 0))
+	    {
+	      error0 = TCP_ERROR_LENGTH;
+	      goto done;
+	    }
+
 	  /* Session exists */
 	  if (PREDICT_TRUE (0 != tc0))
 	    {
@@ -2379,6 +2347,11 @@ tcp46_input_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	      vnet_buffer (b0)->tcp.ack_number =
 		clib_net_to_host_u32 (tcp0->ack_number);
 
+	      vnet_buffer (b0)->tcp.hdr_offset = (u8 *) tcp0
+		- (u8 *) vlib_buffer_get_current (b0);
+	      vnet_buffer (b0)->tcp.data_offset = n_advance_bytes0;
+	      vnet_buffer (b0)->tcp.data_len = n_data_bytes0;
+
 	      flags0 = tcp0->flags & filter_flags;
 	      next0 = tm->dispatch_table[tc0->state][flags0].next;
 	      error0 = tm->dispatch_table[tc0->state][flags0].error;
@@ -2400,14 +2373,14 @@ tcp46_input_inline (vlib_main_t * vm, vlib_node_runtime_t * node,
 	      error0 = TCP_ERROR_NO_LISTENER;
 	    }
 
+	done:
 	  b0->error = error0 ? node->errors[error0] : 0;
 
 	  if (PREDICT_FALSE (b0->flags & VLIB_BUFFER_IS_TRACED))
 	    {
-	      t0 = vlib_add_trace (vm, node, b0, sizeof (*t0));
-	      clib_memcpy (&t0->tcp_header, tcp0, sizeof (t0->tcp_header));
-	      if (tc0)
-		clib_memcpy (&t0->tcp_connection, tc0, sizeof (*tc0));
+	      tcp_rx_trace_t *t0 =
+		vlib_add_trace (vm, node, b0, sizeof (*t0));
+	      tcp_set_rx_trace_data (t0, tc0, tcp0, b0, is_ip4);
 	    }
 
 	  vlib_validate_buffer_enqueue_x1 (vm, node, next_index, to_next,
diff --git a/src/vnet/tcp/tcp_test.c b/src/vnet/tcp/tcp_test.c
index bca5795af12..ed0322062bd 100644
--- a/src/vnet/tcp/tcp_test.c
+++ b/src/vnet/tcp/tcp_test.c
@@ -687,8 +687,7 @@ tcp_test_fifo2 (vlib_main_t * vm)
     {
       tp = vp + i;
       data64 = tp->offset;
-      rv = svm_fifo_enqueue_with_offset (f, tp->offset, tp->len,
-					 (u8 *) & data64);
+      svm_fifo_enqueue_with_offset (f, tp->offset, tp->len, (u8 *) & data64);
     }
 
   /* Expected result: one big fat chunk at offset 4 */
@@ -891,9 +890,9 @@ tcp_test_fifo3 (vlib_main_t * vm, unformat_input_t * input)
   for (i = 0; i < vec_len (generate); i++)
     {
       tp = generate + i;
-      rv = svm_fifo_enqueue_with_offset (f, fifo_initial_offset
-					 + tp->offset, tp->len,
-					 (u8 *) data_pattern + tp->offset);
+      svm_fifo_enqueue_with_offset (f, fifo_initial_offset + tp->offset,
+				    tp->len,
+				    (u8 *) data_pattern + tp->offset);
     }
 
   /*