From d6042d4f1ea0baf02bc87c72960a331a9e08dfab Mon Sep 17 00:00:00 2001 From: Steven Date: Thu, 7 Sep 2017 09:20:56 -0700 Subject: [PATCH] memif: fix coverity warnings as of 9/7 1. coverity complains about "buffer not null terminated" for strncpy because we pass the size of the destination to the call which is equal to the true size of the destination. We subtract 1 for the size to accommodate the null like all other places are already doing it. 2. Add a check to tx_queues in memif_interface_tx_inline to avoid "divide by zero". 3. To avoid null pointer dereference in memif_create_if, change the goto done rather than goto error and spit a more meaningful error rather than silent about it. 4. Shuffle a line to avoid "check after use" in vl_api_memif_delete_t_handler. Change-Id: Icba7ecd5362c012a48ac35795d31aab356617420 Signed-off-by: Steven --- src/plugins/memif/device.c | 14 ++++++++++++-- src/plugins/memif/memif.c | 7 +++++-- src/plugins/memif/memif_api.c | 7 +++++-- src/plugins/memif/socket.c | 13 +++++++------ 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/plugins/memif/device.c b/src/plugins/memif/device.c index c76825349f3..aff18f2df6b 100644 --- a/src/plugins/memif/device.c +++ b/src/plugins/memif/device.c @@ -31,7 +31,8 @@ #define foreach_memif_tx_func_error \ _(NO_FREE_SLOTS, "no free tx slots") \ _(TRUNC_PACKET, "packet > buffer size -- truncated in tx ring") \ -_(PENDING_MSGS, "pending msgs in tx ring") +_(PENDING_MSGS, "pending msgs in tx ring") \ +_(NO_TX_QUEUES, "no tx queues") typedef enum { @@ -169,6 +170,13 @@ memif_interface_tx_inline (vlib_main_t * vm, vlib_node_runtime_t * node, u8 tx_queues = vec_len (mif->tx_queues); memif_queue_t *mq; + if (PREDICT_FALSE (tx_queues == 0)) + { + vlib_error_count (vm, node->node_index, MEMIF_TX_ERROR_NO_TX_QUEUES, + n_left); + goto error; + } + if (tx_queues < vec_len (vlib_mains)) { qid = thread_index % tx_queues; @@ -249,7 +257,6 @@ memif_interface_tx_inline (vlib_main_t * vm, vlib_node_runtime_t * node, n_left); } - vlib_buffer_free (vm, vlib_frame_args (frame), frame->n_vectors); if ((ring->flags & MEMIF_RING_FLAG_MASK_INT) == 0 && mq->int_fd > -1) { u64 b = 1; @@ -257,6 +264,9 @@ memif_interface_tx_inline (vlib_main_t * vm, vlib_node_runtime_t * node, mq->int_count++; } +error: + vlib_buffer_free (vm, vlib_frame_args (frame), frame->n_vectors); + return frame->n_vectors; } diff --git a/src/plugins/memif/memif.c b/src/plugins/memif/memif.c index af81faf2711..7e2d947f426 100644 --- a/src/plugins/memif/memif.c +++ b/src/plugins/memif/memif.c @@ -611,8 +611,11 @@ memif_create_if (vlib_main_t * vm, memif_create_if_args_t * args) } else { - ret = VNET_API_ERROR_SYSCALL_ERROR_3; - goto error; + error = clib_error_return (0, "File exists for %s", + socket_filename); + clib_error_report (error); + rv = VNET_API_ERROR_VALUE_EXIST; + goto done; } } pool_get (mm->socket_files, msf); diff --git a/src/plugins/memif/memif_api.c b/src/plugins/memif/memif_api.c index 2b7b792ae20..89afdb8d681 100644 --- a/src/plugins/memif/memif_api.c +++ b/src/plugins/memif/memif_api.c @@ -201,13 +201,16 @@ vl_api_memif_delete_t_handler (vl_api_memif_delete_t * mp) vl_api_memif_delete_reply_t *rmp; vnet_hw_interface_t *hi = vnet_get_sup_hw_interface (vnm, ntohl (mp->sw_if_index)); - memif_if_t *mif = pool_elt_at_index (mm->interfaces, hi->dev_instance); + memif_if_t *mif; int rv = 0; if (hi == NULL || memif_device_class.index != hi->dev_class_index) rv = VNET_API_ERROR_INVALID_SW_IF_INDEX; else - rv = memif_delete_if (vm, mif); + { + mif = pool_elt_at_index (mm->interfaces, hi->dev_instance); + rv = memif_delete_if (vm, mif); + } REPLY_MACRO (VL_API_MEMIF_DELETE_REPLY); } diff --git a/src/plugins/memif/socket.c b/src/plugins/memif/socket.c index a3c8d544b7d..79ae07bef15 100644 --- a/src/plugins/memif/socket.c +++ b/src/plugins/memif/socket.c @@ -115,7 +115,7 @@ memif_msg_enq_hello (int fd) h->max_region = MEMIF_MAX_REGION; h->max_log2_ring_size = MEMIF_MAX_LOG2_RING_SIZE; s = format (0, "VPP %s%c", VPP_BUILD_VER, 0); - strncpy ((char *) h->name, (char *) s, sizeof (h->name)); + strncpy ((char *) h->name, (char *) s, sizeof (h->name) - 1); vec_free (s); return memif_msg_send (fd, &msg, -1); } @@ -134,9 +134,10 @@ memif_msg_enq_init (memif_if_t * mif) i->id = mif->id; i->mode = mif->mode; s = format (0, "VPP %s%c", VPP_BUILD_VER, 0); - strncpy ((char *) i->name, (char *) s, sizeof (i->name)); + strncpy ((char *) i->name, (char *) s, sizeof (i->name) - 1); if (mif->secret) - strncpy ((char *) i->secret, (char *) mif->secret, sizeof (i->secret)); + strncpy ((char *) i->secret, (char *) mif->secret, + sizeof (i->secret) - 1); vec_free (s); } @@ -189,7 +190,7 @@ memif_msg_enq_connect (memif_if_t * mif) e->msg.type = MEMIF_MSG_TYPE_CONNECT; e->fd = -1; s = format (0, "%U%c", format_memif_device_name, mif->dev_instance, 0); - strncpy ((char *) c->if_name, (char *) s, sizeof (c->if_name)); + strncpy ((char *) c->if_name, (char *) s, sizeof (c->if_name) - 1); vec_free (s); } @@ -204,7 +205,7 @@ memif_msg_enq_connected (memif_if_t * mif) e->msg.type = MEMIF_MSG_TYPE_CONNECTED; e->fd = -1; s = format (0, "%U%c", format_memif_device_name, mif->dev_instance, 0); - strncpy ((char *) c->if_name, (char *) s, sizeof (c->if_name)); + strncpy ((char *) c->if_name, (char *) s, sizeof (c->if_name) - 1); vec_free (s); } @@ -216,7 +217,7 @@ memif_msg_send_disconnect (memif_if_t * mif, clib_error_t * err) memif_msg_disconnect_t *d = &msg.disconnect; d->code = err->code; - strncpy ((char *) d->string, (char *) err->what, sizeof (d->string)); + strncpy ((char *) d->string, (char *) err->what, sizeof (d->string) - 1); return memif_msg_send (mif->conn_fd, &msg, -1); } -- 2.16.6