From f06dfe03976064a1d770f9d1dd06b496f20f90e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 25 Nov 2020 12:45:47 +0100 Subject: [PATCH 1/4] Remove cppcheck 2.0 false positive workarounds The cppcheck bug which commit 481fa34e50a6183273f71175adf93bfb12cad1e9 works around was fixed in cppcheck 2.2. Drop the relevant hack from the definition of the cppcheck GitLab CI job. --- .gitlab-ci.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e4b37358b2..5ef3fd83b0 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -422,9 +422,6 @@ stages: <<: *default_triggering_rules stage: postcheck script: - # Workaround for cppcheck 2.0 uninitvar false positives triggered by (&var)->field syntax - # (see: https://sourceforge.net/p/cppcheck/discussion/general/thread/122153e3c1/) - - sed -i '/^#define ISC__BUFFER.*\\$/{s|_b|__b|;N;s|do {|\0 isc_buffer_t *_b = (isc_buffer_t *)__b;|}; /^#define ISC__BUFFER.*REGION.*\\$/{s|_r|__r|;N;s|do {|\0 isc_region_t *_r = (isc_region_t *)__r;|; /USEDREGION/{s|isc_buffer_t|const \0|g}}' lib/isc/include/isc/buffer.h - *configure - (make -nwk all || true) | compiledb - export GCC_VERSION=$(gcc --version | sed -n 's/.* \([0-9]\+\)\.[0-9]\+\.[0-9]\+.*/\1/p') From d9701e22b5b98fbc9915c319eb364a56652979bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 25 Nov 2020 12:45:47 +0100 Subject: [PATCH 2/4] Teach cppcheck that fatal() does not return cppcheck is not aware that the bin/dnssec/dnssectool.c:fatal() function does not return. This triggers certain cppcheck 2.2 false positives, for example: bin/dnssec/dnssec-signzone.c:3471:13: warning: Either the condition 'ndskeys==8' is redundant or the array 'dskeyfile[8]' is accessed at index 8, which is out of bounds. [arrayIndexOutOfBoundsCond] dskeyfile[ndskeys++] = isc_commandline_argument; ^ bin/dnssec/dnssec-signzone.c:3468:16: note: Assuming that condition 'ndskeys==8' is not redundant if (ndskeys == MAXDSKEYS) { ^ bin/dnssec/dnssec-signzone.c:3471:13: note: Array index out of bounds dskeyfile[ndskeys++] = isc_commandline_argument; ^ bin/dnssec/dnssec-signzone.c:772:20: warning: Either the condition 'l->hashbuf==NULL' is redundant or there is pointer arithmetic with NULL pointer. [nullPointerArithmeticRedundantCheck] memset(l->hashbuf + l->entries * l->length, 0, l->length); ^ bin/dnssec/dnssec-signzone.c:768:18: note: Assuming that condition 'l->hashbuf==NULL' is not redundant if (l->hashbuf == NULL) { ^ bin/dnssec/dnssec-signzone.c:772:20: note: Null pointer addition memset(l->hashbuf + l->entries * l->length, 0, l->length); ^ Instead of suppressing all such warnings individually, conditionally define a preprocessor macro which prevents them from being triggered. --- bin/dnssec/dnssec-keyfromlabel.c | 4 ---- bin/dnssec/dnssec-keygen.c | 2 -- bin/dnssec/dnssectool.h | 4 ++++ 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/bin/dnssec/dnssec-keyfromlabel.c b/bin/dnssec/dnssec-keyfromlabel.c index 81c6cc1037..bb94c98a18 100644 --- a/bin/dnssec/dnssec-keyfromlabel.c +++ b/bin/dnssec/dnssec-keyfromlabel.c @@ -362,7 +362,6 @@ main(int argc, char **argv) { setup_logging(mctx, &log); if (predecessor == NULL) { - /* cppcheck-suppress nullPointerRedundantCheck */ if (label == NULL) { fatal("the key label was not specified"); } @@ -384,7 +383,6 @@ main(int argc, char **argv) { isc_result_totext(ret)); } - /* cppcheck-suppress nullPointerRedundantCheck */ if (strchr(label, ':') == NULL) { char *l; int len; @@ -396,13 +394,11 @@ main(int argc, char **argv) { label = l; } - /* cppcheck-suppress nullPointerRedundantCheck */ if (algname == NULL) { fatal("no algorithm specified"); } r.base = algname; - /* cppcheck-suppress nullPointerRedundantCheck */ r.length = strlen(algname); ret = dns_secalg_fromtext(&alg, &r); if (ret != ISC_R_SUCCESS) { diff --git a/bin/dnssec/dnssec-keygen.c b/bin/dnssec/dnssec-keygen.c index 30e16fb87f..f16d98257b 100644 --- a/bin/dnssec/dnssec-keygen.c +++ b/bin/dnssec/dnssec-keygen.c @@ -1180,12 +1180,10 @@ main(int argc, char **argv) { } if (ctx.predecessor == NULL && ctx.policy == NULL) { - /* cppcheck-suppress nullPointerRedundantCheck */ if (algname == NULL) { fatal("no algorithm specified"); } r.base = algname; - /* cppcheck-suppress nullPointerRedundantCheck */ r.length = strlen(algname); ret = dns_secalg_fromtext(&ctx.alg, &r); if (ret != ISC_R_SUCCESS) { diff --git a/bin/dnssec/dnssectool.h b/bin/dnssec/dnssectool.h index bb18c69183..e0970e2ae6 100644 --- a/bin/dnssec/dnssectool.h +++ b/bin/dnssec/dnssectool.h @@ -43,8 +43,12 @@ extern uint8_t dtype[8]; typedef void(fatalcallback_t)(void); +#ifndef CPPCHECK ISC_NORETURN void fatal(const char *format, ...) ISC_FORMAT_PRINTF(1, 2); +#else /* CPPCHECK */ +#define fatal(...) exit(1) +#endif void setfatalcallback(fatalcallback_t *callback); From 0b6216d1c702ef0aaa66a2a1bea70cfb12c81468 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 25 Nov 2020 12:45:47 +0100 Subject: [PATCH 3/4] Silence cppcheck 2.2 false positive in udp_recv() cppcheck 2.2 reports the following false positive: lib/dns/dispatch.c:1239:14: warning: Either the condition 'resp==NULL' is redundant or there is possible null pointer dereference: resp. [nullPointerRedundantCheck] if (disp != resp->disp) { ^ lib/dns/dispatch.c:1210:11: note: Assuming that condition 'resp==NULL' is not redundant if (resp == NULL) { ^ lib/dns/dispatch.c:1239:14: note: Null pointer dereference if (disp != resp->disp) { ^ Apparently this version of cppcheck gets confused about conditional "goto" statements because line 1239 can never be reached if 'resp' is NULL. Move a code block to prevent the above false positive from being reported without affecting the processing logic. --- lib/dns/dispatch.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index 82c019168d..f226d156bf 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -1217,11 +1217,6 @@ udp_recv(isc_event_t *ev_in, dns_dispatch_t *disp, dispsocket_t *dispsock) { "search for response in bucket %d: %s", bucket, (resp == NULL ? "not found" : "found")); - if (resp == NULL) { - inc_stats(mgr, dns_resstatscounter_mismatch); - free_buffer(disp, ev->region.base, ev->region.length); - goto unlock; - } } else if (resp->id != id || !isc_sockaddr_equal(&ev->address, &resp->host)) { dispatch_log(disp, LVL(90), @@ -1231,6 +1226,12 @@ udp_recv(isc_event_t *ev_in, dns_dispatch_t *disp, dispsocket_t *dispsock) { goto unlock; } + if (resp == NULL) { + inc_stats(mgr, dns_resstatscounter_mismatch); + free_buffer(disp, ev->region.base, ev->region.length); + goto unlock; + } + /* * Now that we have the original dispatch the query was sent * from check that the address and port the response was From ea54a932d225fc55d2a815d200c3f3b988675709 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 25 Nov 2020 12:45:47 +0100 Subject: [PATCH 4/4] Convert add_quota() to a function cppcheck 2.2 reports the following false positive: lib/isc/tests/quota_test.c:71:21: error: Array 'quotas[101]' accessed at index 110, which is out of bounds. [arrayIndexOutOfBounds] isc_quota_t *quotas[110]; ^ The above is not even an array access, so this report is obviously caused by a cppcheck bug. Yet, it seems to be triggered by the presence of the add_quota() macro, which should really be a function. Convert the add_quota() macro to a function in order to make the code cleaner and to prevent the above cppcheck 2.2 false positive from being triggered. --- lib/isc/tests/quota_test.c | 46 +++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/lib/isc/tests/quota_test.c b/lib/isc/tests/quota_test.c index 36b661c3c8..99ba08f75b 100644 --- a/lib/isc/tests/quota_test.c +++ b/lib/isc/tests/quota_test.c @@ -52,19 +52,29 @@ isc_quota_get_set_test(void **state) { isc_quota_destroy("a); } -#define add_quota(quota, quotasp, exp, attached, exp_used) \ - { \ - *quotasp = NULL; \ - isc_result_t result = isc_quota_attach(quota, quotasp); \ - assert_int_equal(result, exp); \ - if (attached) { \ - assert_ptr_equal(*quotasp, quota); \ - } else { \ - assert_null(*quotasp); \ - } \ - assert_int_equal(isc_quota_getused(quota), exp_used); \ +static void +add_quota(isc_quota_t *source, isc_quota_t **target, + isc_result_t expected_result, int expected_used) { + isc_result_t result; + + *target = NULL; + + result = isc_quota_attach(source, target); + assert_int_equal(result, expected_result); + + switch (expected_result) { + case ISC_R_SUCCESS: + case ISC_R_SOFTQUOTA: + assert_ptr_equal(*target, source); + break; + default: + assert_null(*target); + break; } + assert_int_equal(isc_quota_getused(source), expected_used); +} + static void isc_quota_hard_test(void **state) { isc_quota_t quota; @@ -75,18 +85,18 @@ isc_quota_hard_test(void **state) { isc_quota_init("a, 100); for (i = 0; i < 100; i++) { - add_quota("a, "as[i], ISC_R_SUCCESS, true, i + 1); + add_quota("a, "as[i], ISC_R_SUCCESS, i + 1); } - add_quota("a, "as[100], ISC_R_QUOTA, false, 100); + add_quota("a, "as[100], ISC_R_QUOTA, 100); assert_int_equal(isc_quota_getused("a), 100); isc_quota_detach("as[0]); assert_null(quotas[0]); - add_quota("a, "as[100], ISC_R_SUCCESS, true, 100); - add_quota("a, "as[101], ISC_R_QUOTA, false, 100); + add_quota("a, "as[100], ISC_R_SUCCESS, 100); + add_quota("a, "as[101], ISC_R_QUOTA, 100); for (i = 100; i > 0; i--) { isc_quota_detach("as[i]); @@ -108,13 +118,13 @@ isc_quota_soft_test(void **state) { isc_quota_soft("a, 50); for (i = 0; i < 50; i++) { - add_quota("a, "as[i], ISC_R_SUCCESS, true, i + 1); + add_quota("a, "as[i], ISC_R_SUCCESS, i + 1); } for (i = 50; i < 100; i++) { - add_quota("a, "as[i], ISC_R_SOFTQUOTA, true, i + 1); + add_quota("a, "as[i], ISC_R_SOFTQUOTA, i + 1); } - add_quota("a, "as[i], ISC_R_QUOTA, false, 100); + add_quota("a, "as[i], ISC_R_QUOTA, 100); for (i = 99; i >= 0; i--) { isc_quota_detach("as[i]);