From 72d54b68a69f441040388f2fbd42c83107741197 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Wed, 15 Jan 2020 16:09:18 +0100 Subject: [PATCH 1/7] lwip: dhcp-server fix static analysis warnings 1) kill_oldest_dhcps_pool() is only called when list has at least two members (assured with kconfig value limit), added assertion to ensure this function is used only when prerequisities are met 2) use after free reported in two places, since the analyzer checks also the scenario when the linked list has loops, added ignore tags --- components/lwip/apps/dhcpserver/dhcpserver.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/components/lwip/apps/dhcpserver/dhcpserver.c b/components/lwip/apps/dhcpserver/dhcpserver.c index 5052b67414..03c17ad9d3 100644 --- a/components/lwip/apps/dhcpserver/dhcpserver.c +++ b/components/lwip/apps/dhcpserver/dhcpserver.c @@ -253,11 +253,13 @@ void node_remove_from_list(list_node **phead, list_node *pdelete) *phead = NULL; } else { if (plist == pdelete) { - *phead = plist->pnext; + // Note: Ignoring the "use after free" warnings, as it could only happen + // if the linked list contains loops + *phead = plist->pnext; // NOLINT(clang-analyzer-unix.Malloc) pdelete->pnext = NULL; } else { while (plist != NULL) { - if (plist->pnext == pdelete) { + if (plist->pnext == pdelete) { // NOLINT(clang-analyzer-unix.Malloc) plist->pnext = pdelete->pnext; pdelete->pnext = NULL; } @@ -1216,6 +1218,7 @@ static void kill_oldest_dhcps_pool(void) list_node *minpre = NULL, *minp = NULL; struct dhcps_pool *pdhcps_pool = NULL, *pmin_pool = NULL; pre = plist; + assert(pre != NULL && pre->pnext != NULL); // Expect the list to have at least 2 nodes p = pre->pnext; minpre = pre; minp = p; From a2d55ddece3cd2c7320abc5090271af6333531c6 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Mon, 21 Oct 2019 09:52:32 +0200 Subject: [PATCH 2/7] ci: update static analysis rules to fail on any new issue zeroing out limits for number of warnings which are tolerated closes IDF-973 --- tools/ci/static-analysis-rules.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/ci/static-analysis-rules.yml b/tools/ci/static-analysis-rules.yml index 6678b85cb3..d96b0a0aa9 100644 --- a/tools/ci/static-analysis-rules.yml +++ b/tools/ci/static-analysis-rules.yml @@ -1,6 +1,6 @@ limits: - "clang-analyzer-core.NullDereference" : 9 - "clang-analyzer-unix.Malloc" : 9 + "clang-analyzer-core.NullDereference" : 0 + "clang-analyzer-unix.Malloc" : 0 ignore: - "llvm-header-guard" @@ -12,7 +12,7 @@ skip: - "components/asio/asio" - "components/bootloader/subproject/components/micro-ecc/micro-ecc" - "components/bt/lib" - - "components/coap/libcoap" + - "components/coap" - "components/esp_wifi/lib_esp32" - "components/expat/expat" - "components/json/cJSON" From 9b821ddd6b792ef897ef55415b36fa2c1cf68771 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Mon, 21 Oct 2019 15:34:34 +0200 Subject: [PATCH 3/7] sdmmc: fix possible null dereference in output parameter assignement, whilst it was null checked as an input parameter --- components/sdmmc/sdmmc_io.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/components/sdmmc/sdmmc_io.c b/components/sdmmc/sdmmc_io.c index b7611f8536..60ef6e947d 100644 --- a/components/sdmmc/sdmmc_io.c +++ b/components/sdmmc/sdmmc_io.c @@ -567,6 +567,9 @@ esp_err_t sdmmc_io_get_cis_data(sdmmc_card_t* card, uint8_t* out_buffer, size_t esp_err_t ret = ESP_OK; WORD_ALIGNED_ATTR uint8_t buf[CIS_GET_MINIMAL_SIZE]; + /* Pointer to size is a mandatory parameter */ + assert(inout_cis_size); + /* * CIS region exist in 0x1000~0x17FFF of FUNC 0, get the start address of it * from CCCR register. @@ -585,7 +588,7 @@ esp_err_t sdmmc_io_get_cis_data(sdmmc_card_t* card, uint8_t* out_buffer, size_t * existing. */ size_t max_reading = UINT32_MAX; - if (inout_cis_size && *inout_cis_size != 0) { + if (*inout_cis_size != 0) { max_reading = *inout_cis_size; } From 2e28ab29c7203095674c5213c3e536b75e00c0cc Mon Sep 17 00:00:00 2001 From: David Cermak Date: Mon, 4 Nov 2019 16:17:02 +0100 Subject: [PATCH 4/7] freertos: silence the static analysis warning referencing the workitem --- components/freertos/tasks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index c057cb4c0c..5290168542 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -649,7 +649,7 @@ void taskYIELD_OTHER_CORE( BaseType_t xCoreID, UBaseType_t uxPriority ) BaseType_t i; if (xCoreID != tskNO_AFFINITY) { - if ( curTCB->uxPriority < uxPriority ) { + if ( curTCB->uxPriority < uxPriority ) { // NOLINT(clang-analyzer-core.NullDereference) IDF-685 vPortYieldOtherCore( xCoreID ); } } From 62f9f42b542e7da63c784eebc62f1b310fb01041 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Fri, 27 Mar 2020 19:17:36 +0100 Subject: [PATCH 5/7] wpa_supplicant: ignore static analysis violations --- components/wpa_supplicant/src/ap/wpa_auth.c | 2 +- components/wpa_supplicant/src/esp_supplicant/esp_wpa2.c | 2 +- components/wpa_supplicant/src/wps/wps.c | 2 +- components/wpa_supplicant/src/wps/wps_registrar.c | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/components/wpa_supplicant/src/ap/wpa_auth.c b/components/wpa_supplicant/src/ap/wpa_auth.c index 58625bb553..8b43098790 100644 --- a/components/wpa_supplicant/src/ap/wpa_auth.c +++ b/components/wpa_supplicant/src/ap/wpa_auth.c @@ -2036,7 +2036,7 @@ SM_STATE(WPA_PTK_GROUP, REKEYNEGOTIATING) (!sm->Pair ? WPA_KEY_INFO_INSTALL : 0), rsc, gsm->GNonce, kde, pos - kde, gsm->GN, 1); if (sm->wpa == WPA_VERSION_WPA2) - os_free(kde); + os_free(kde); // NOLINT(clang-analyzer-unix.Malloc) } diff --git a/components/wpa_supplicant/src/esp_supplicant/esp_wpa2.c b/components/wpa_supplicant/src/esp_supplicant/esp_wpa2.c index 7d0ab8cd10..3a7949adaf 100644 --- a/components/wpa_supplicant/src/esp_supplicant/esp_wpa2.c +++ b/components/wpa_supplicant/src/esp_supplicant/esp_wpa2.c @@ -752,7 +752,7 @@ static int eap_peer_sm_init(void) s_wpa2_data_lock = xSemaphoreCreateRecursiveMutex(); if (!s_wpa2_data_lock) { - wpa_printf(MSG_ERROR, "wpa2 eap_peer_sm_init: failed to alloc data lock"); + wpa_printf(MSG_ERROR, "wpa2 eap_peer_sm_init: failed to alloc data lock"); // NOLINT(clang-analyzer-unix.Malloc) return ESP_ERR_NO_MEM; } diff --git a/components/wpa_supplicant/src/wps/wps.c b/components/wpa_supplicant/src/wps/wps.c index b19e187a46..51c728b681 100644 --- a/components/wpa_supplicant/src/wps/wps.c +++ b/components/wpa_supplicant/src/wps/wps.c @@ -275,7 +275,7 @@ int wps_ap_priority_compar(const struct wpabuf *wps_a, } if (wps_a == NULL || wps_parse_msg(wps_a, attr_a) < 0) - return 1; + return 1; // NOLINT(clang-analyzer-unix.Malloc) if (wps_b == NULL || wps_parse_msg(wps_b, attr_b) < 0) return -1; diff --git a/components/wpa_supplicant/src/wps/wps_registrar.c b/components/wpa_supplicant/src/wps/wps_registrar.c index 33f0859b20..76eb2ea63c 100644 --- a/components/wpa_supplicant/src/wps/wps_registrar.c +++ b/components/wpa_supplicant/src/wps/wps_registrar.c @@ -1644,11 +1644,11 @@ int wps_build_cred(struct wps_data *wps, struct wpabuf *msg) return -1; wps->new_psk_len--; /* remove newline */ while (wps->new_psk_len && - wps->new_psk[wps->new_psk_len - 1] == '=') + wps->new_psk[wps->new_psk_len - 1] == '=') // NOLINT(clang-analyzer-unix.Malloc) wps->new_psk_len--; wpa_hexdump_ascii_key(MSG_DEBUG, "WPS: Generated passphrase", wps->new_psk, wps->new_psk_len); - os_memcpy(wps->cred.key, wps->new_psk, wps->new_psk_len); + os_memcpy(wps->cred.key, wps->new_psk, wps->new_psk_len); // NOLINT(clang-analyzer-unix.Malloc) wps->cred.key_len = wps->new_psk_len; } else if (wps->use_psk_key && wps->wps->psk_set) { char hex[65]; From 06c46837ce18ddf97881523415118697b3f1abcf Mon Sep 17 00:00:00 2001 From: David Cermak Date: Fri, 27 Mar 2020 19:18:40 +0100 Subject: [PATCH 6/7] panic: ignore deliberate null dereference to pass static analysis --- components/esp_system/panic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/esp_system/panic.c b/components/esp_system/panic.c index 628a153892..631a656495 100644 --- a/components/esp_system/panic.c +++ b/components/esp_system/panic.c @@ -338,6 +338,6 @@ void __attribute__((noreturn)) panic_abort(const char *details) #endif #endif - *((int *) 0) = 0; // should be an invalid operation on targets + *((int *) 0) = 0; // NOLINT(clang-analyzer-core.NullDereference) should be an invalid operation on targets while(1); } \ No newline at end of file From 585633b254e9d9c0f532dcdd6a7542d71eb57f4a Mon Sep 17 00:00:00 2001 From: David Cermak Date: Fri, 27 Mar 2020 19:19:20 +0100 Subject: [PATCH 7/7] console: ignore static analysis warnings --- components/console/argtable3/argtable3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/console/argtable3/argtable3.c b/components/console/argtable3/argtable3.c index 43464d396e..65abe75631 100644 --- a/components/console/argtable3/argtable3.c +++ b/components/console/argtable3/argtable3.c @@ -3007,7 +3007,7 @@ static int trex_newnode(TRex *exp, TRexNodeType type) exp->_nallocated *= 2; exp->_nodes = (TRexNode *)realloc(exp->_nodes, exp->_nallocated * sizeof(TRexNode)); } - exp->_nodes[exp->_nsize++] = n; + exp->_nodes[exp->_nsize++] = n; // NOLINT(clang-analyzer-unix.Malloc) newid = exp->_nsize - 1; return (int)newid; }