From b2bcad6238b7e8a669ae29c74079eb9bb9fbb694 Mon Sep 17 00:00:00 2001 From: Chris Luke Date: Mon, 18 Sep 2017 08:51:22 -0400 Subject: [PATCH] Fixes for issues Coverity has reported (VPP-972) 177117: fstat() returns -1 on error; the code is checking for any positive value instead 175142: final return could never be reached; simple refactoring 175235,175236: Warning suppressed with an explicit cast to (void) 174817: Final return couldn't be reached; is is_in_order is 0 then 'rv' is already returned above 172095,172093: If is_is_set does not get set to 1, then return 0 has already been invoked 174405: Re-kill this (nothing sets rv) 171136: Looks like a cmd line flag to set test_bytes was missing; added it, and refactored the argc/argv processing to avoid two other potential segv's 176813: Add range checking for term width/height. First stab at a reasonable range is 1-512 for both. 175350: Fix implicit casting in shift operation 174272: Not a c+p error; try using a coverity annotation to ignore it 174273,175320: Annotated FORWARD_NULL Change-Id: I58d0f860fc2209f59f8d1b6b344d631b8d429ace Signed-off-by: Chris Luke --- src/uri/sock_test_client.c | 1 + src/uri/uri_socket_test.c | 24 ++++++++++++++++++++++-- src/uri/vppcom.c | 8 ++++++-- src/vlib/linux/physmem.c | 2 +- src/vlib/unix/cli.c | 30 ++++++++++++++++++++++++++++++ src/vnet/mpls/mpls_api.c | 2 +- src/vnet/session/session.c | 5 +---- src/vnet/session/session_cli.c | 37 +++++++------------------------------ src/vnet/session/session_lookup.c | 8 ++++---- src/vppinfra/linux/mem.c | 2 +- 10 files changed, 74 insertions(+), 45 deletions(-) diff --git a/src/uri/sock_test_client.c b/src/uri/sock_test_client.c index ab8e5a0e4a7..151c90b2960 100644 --- a/src/uri/sock_test_client.c +++ b/src/uri/sock_test_client.c @@ -429,6 +429,7 @@ exit_client (void) tsock = &scm->test_socket[i]; tsock->cfg.test = SOCK_TEST_TYPE_EXIT; + /* coverity[COPY_PASTE_ERROR] */ if (ctrl->cfg.verbose) { printf ("\nCLIENT (fd %d): Sending exit cfg to server...\n", diff --git a/src/uri/uri_socket_test.c b/src/uri/uri_socket_test.c index 5f7084d5b20..4469b03d4c2 100644 --- a/src/uri/uri_socket_test.c +++ b/src/uri/uri_socket_test.c @@ -36,8 +36,6 @@ main (int argc, char *argv[]) if (argc >= 3) { - bytes = ((long) atoi (argv[4])) << 20; - no_echo = atoi (argv[3]); portno = atoi (argv[2]); server = gethostbyname (argv[1]); if (server == NULL) @@ -45,6 +43,28 @@ main (int argc, char *argv[]) clib_unix_warning ("gethostbyname"); exit (1); } + + argc -= 3; + argv += 3; + + if (argc) + { + bytes = ((long) atoi (argv[0])) << 20; + argc--; + argv++; + } + if (argc) + { + no_echo = atoi (argv[0]); + argc--; + argv++; + } + if (argc) + { + test_bytes = atoi (argv[0]); + argc--; + argv++; + } } else { diff --git a/src/uri/vppcom.c b/src/uri/vppcom.c index 8a8a806caf0..c7ae0ea5eff 100644 --- a/src/uri/vppcom.c +++ b/src/uri/vppcom.c @@ -1478,7 +1478,7 @@ vppcom_cfg_read (char *conf_fname) while (unformat_check_input (input) != UNFORMAT_END_OF_INPUT) { - unformat_user (input, unformat_line_input, line_input); + (void) unformat_user (input, unformat_line_input, line_input); unformat_skip_white_space (line_input); if (unformat (line_input, "vppcom {")) @@ -2359,12 +2359,14 @@ vppcom_select (unsigned long n_bits, unsigned long *read_map, clib_bitmap_get (vcm->ex_bitmap, session_index) && (rv < 0)) { // TBD: clib_warning + /* coverity[FORWARD_NULL] */ clib_bitmap_set_no_check (except_map, session_index, 1); bits_set++; } else if (rv > 0) { // TBD: clib_warning + /* coverity[FORWARD_NULL] */ clib_bitmap_set_no_check (read_map, session_index, 1); bits_set++; } @@ -2387,9 +2389,10 @@ vppcom_select (unsigned long n_bits, unsigned long *read_map, rv = vppcom_session_write_ready (session, session_index); clib_spinlock_unlock (&vcm->sessions_lockp); - if (rv > 0) + if (rv > 0 ) { // TBD: clib_warning + /* coverity[FORWARD_NULL] */ clib_bitmap_set_no_check (write_map, session_index, 1); bits_set++; } @@ -2415,6 +2418,7 @@ vppcom_select (unsigned long n_bits, unsigned long *read_map, if (rv < 0) { // TBD: clib_warning + /* coverity[FORWARD_NULL] */ clib_bitmap_set_no_check (except_map, session_index, 1); bits_set++; } diff --git a/src/vlib/linux/physmem.c b/src/vlib/linux/physmem.c index 3cc42a0676d..6d3f7c55d32 100644 --- a/src/vlib/linux/physmem.c +++ b/src/vlib/linux/physmem.c @@ -157,7 +157,7 @@ unix_physmem_region_alloc (vlib_main_t * vm, char *name, u32 size, pr->mem = alloc.addr; pr->log2_page_size = alloc.log2_page_size; pr->n_pages = alloc.n_pages; - pr->size = pr->n_pages << pr->log2_page_size; + pr->size = (u64) pr->n_pages << (u64) pr->log2_page_size; pr->page_mask = (1 << pr->log2_page_size) - 1; pr->numa_node = numa_node; pr->name = format (0, "%s", name); diff --git a/src/vlib/unix/cli.c b/src/vlib/unix/cli.c index 1567cc2ac0a..1624ce387fb 100644 --- a/src/vlib/unix/cli.c +++ b/src/vlib/unix/cli.c @@ -91,6 +91,15 @@ * protocol message. This is a saftey measure. */ #define UNIX_CLI_MAX_DEPTH_TELNET 24 +/** Minimum terminal width we will accept */ +#define UNIX_CLI_MIN_TERMINAL_WIDTH 1 +/** Maximum terminal width we will accept */ +#define UNIX_CLI_MAX_TERMINAL_WIDTH 512 +/** Minimum terminal height we will accept */ +#define UNIX_CLI_MIN_TERMINAL_HEIGHT 1 +/** Maximum terminal height we will accept */ +#define UNIX_CLI_MAX_TERMINAL_HEIGHT 512 + /** Unix standard in */ #define UNIX_CLI_STDIN_FD 0 @@ -1164,10 +1173,21 @@ unix_cli_process_telnet (unix_main_t * um, /* Window size */ if (i != 8) /* check message is correct size */ break; + cf->width = clib_net_to_host_u16 (*((u16 *) (input_vector + 3))); + if (cf->width > UNIX_CLI_MAX_TERMINAL_WIDTH) + cf->width = UNIX_CLI_MAX_TERMINAL_WIDTH; + if (cf->width < UNIX_CLI_MIN_TERMINAL_WIDTH) + cf->width = UNIX_CLI_MIN_TERMINAL_WIDTH; + cf->height = clib_net_to_host_u16 (*((u16 *) (input_vector + 5))); + if (cf->height > UNIX_CLI_MAX_TERMINAL_HEIGHT) + cf->height = UNIX_CLI_MAX_TERMINAL_HEIGHT; + if (cf->height < UNIX_CLI_MIN_TERMINAL_HEIGHT) + cf->height = UNIX_CLI_MIN_TERMINAL_HEIGHT; + /* reindex pager buffer */ unix_cli_pager_reindex (cf); /* redraw page */ @@ -2539,8 +2559,18 @@ unix_cli_resize_interrupt (int signum) /* We can't trust ws.XXX... */ return; } + cf->width = ws.ws_col; + if (cf->width > UNIX_CLI_MAX_TERMINAL_WIDTH) + cf->width = UNIX_CLI_MAX_TERMINAL_WIDTH; + if (cf->width < UNIX_CLI_MIN_TERMINAL_WIDTH) + cf->width = UNIX_CLI_MIN_TERMINAL_WIDTH; + cf->height = ws.ws_row; + if (cf->height > UNIX_CLI_MAX_TERMINAL_HEIGHT) + cf->height = UNIX_CLI_MAX_TERMINAL_HEIGHT; + if (cf->height < UNIX_CLI_MIN_TERMINAL_HEIGHT) + cf->height = UNIX_CLI_MIN_TERMINAL_HEIGHT; /* Reindex the pager buffer */ unix_cli_pager_reindex (cf); diff --git a/src/vnet/mpls/mpls_api.c b/src/vnet/mpls/mpls_api.c index 988c2c98232..762c40ffa8f 100644 --- a/src/vnet/mpls/mpls_api.c +++ b/src/vnet/mpls/mpls_api.c @@ -96,7 +96,7 @@ vl_api_mpls_table_add_del_t_handler (vl_api_mpls_table_add_del_t * mp) else mpls_table_delete (ntohl (mp->mt_table_id), 1); - rv = (rv == 0) ? vnm->api_errno : rv; + // NB: Nothing sets rv; none of the above returns an error REPLY_MACRO (VL_API_MPLS_TABLE_ADD_DEL_REPLY); } diff --git a/src/vnet/session/session.c b/src/vnet/session/session.c index 4544f9a0f93..792e6612dc1 100644 --- a/src/vnet/session/session.c +++ b/src/vnet/session/session.c @@ -267,10 +267,7 @@ stream_session_enqueue_data (transport_connection_t * tc, vlib_buffer_t * b, } } - if (is_in_order) - return enqueued; - - return 0; + return enqueued; } /** Check if we have space in rx fifo to push more bytes */ diff --git a/src/vnet/session/session_cli.c b/src/vnet/session/session_cli.c index d9f516beab9..8c30a1df7eb 100755 --- a/src/vnet/session/session_cli.c +++ b/src/vnet/session/session_cli.c @@ -127,13 +127,8 @@ unformat_stream_session_id (unformat_input_t * input, va_list * args) *is_ip4 = 0; tuple_is_set = 1; } - else - return 0; - - if (tuple_is_set) - return 1; - return 0; + return tuple_is_set; } uword @@ -144,21 +139,12 @@ unformat_stream_session (unformat_input_t * input, va_list * args) u8 proto = ~0; ip46_address_t lcl, rmt; u32 lcl_port = 0, rmt_port = 0; - u8 is_ip4 = 0, s_type = ~0, id_is_set = 0; + u8 is_ip4 = 0, s_type = ~0; - if (unformat (input, "%U", unformat_stream_session_id, &proto, &lcl, &rmt, - &lcl_port, &rmt_port, &is_ip4)) - { - id_is_set = 1; - } - else + if (!unformat (input, "%U", unformat_stream_session_id, &proto, &lcl, &rmt, + &lcl_port, &rmt_port, &is_ip4)) return 0; - if (!id_is_set) - { - return 0; - } - s_type = session_type_from_proto_and_ip (proto, is_ip4); if (is_ip4) s = stream_session_lookup4 (&lcl.ip4, &rmt.ip4, @@ -185,21 +171,12 @@ unformat_transport_connection (unformat_input_t * input, va_list * args) u8 proto = ~0; ip46_address_t lcl, rmt; u32 lcl_port = 0, rmt_port = 0; - u8 is_ip4 = 0, s_type = ~0, id_is_set = 0; + u8 is_ip4 = 0, s_type = ~0; - if (unformat (input, "%U", unformat_stream_session_id, &proto, &lcl, &rmt, - &lcl_port, &rmt_port, &is_ip4)) - { - id_is_set = 1; - } - else + if (!unformat (input, "%U", unformat_stream_session_id, &proto, &lcl, &rmt, + &lcl_port, &rmt_port, &is_ip4)) return 0; - if (!id_is_set) - { - return 0; - } - proto = (proto == (u8) ~ 0) ? suggested_proto : proto; if (proto == (u8) ~ 0) return 0; diff --git a/src/vnet/session/session_lookup.c b/src/vnet/session/session_lookup.c index 0f9abf9aa74..4487b1c305f 100644 --- a/src/vnet/session/session_lookup.c +++ b/src/vnet/session/session_lookup.c @@ -233,15 +233,15 @@ stream_session_half_open_table_add (transport_connection_t * tc, u64 value) { make_v4_ss_kv_from_tc (&kv4, tc); kv4.value = value; - clib_bihash_add_del_16_8 (&sl->v4_half_open_hash, &kv4, - 1 /* is_add */ ); + (void) clib_bihash_add_del_16_8 (&sl->v4_half_open_hash, &kv4, + 1 /* is_add */ ); } else { make_v6_ss_kv_from_tc (&kv6, tc); kv6.value = value; - clib_bihash_add_del_48_8 (&sl->v6_half_open_hash, &kv6, - 1 /* is_add */ ); + (void) clib_bihash_add_del_48_8 (&sl->v6_half_open_hash, &kv6, + 1 /* is_add */ ); } } diff --git a/src/vppinfra/linux/mem.c b/src/vppinfra/linux/mem.c index 665ddf6194f..df46763ab2c 100644 --- a/src/vppinfra/linux/mem.c +++ b/src/vppinfra/linux/mem.c @@ -49,7 +49,7 @@ int clib_mem_vm_get_log2_page_size (int fd) { struct stat st = { 0 }; - if (fstat (fd, &st)) + if (fstat (fd, &st) == -1) return 0; return min_log2 (st.st_blksize); } -- 2.16.6