From 668a972d1e71ccc7282601906dff4bc4bb3a7d1c Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 30 Jan 2020 17:14:42 +1100 Subject: [PATCH 01/13] simplify RUNTIME_CHECK for cppcheck --- lib/isc/include/isc/util.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/isc/include/isc/util.h b/lib/isc/include/isc/util.h index c4a872cba7..20f107b8c6 100644 --- a/lib/isc/include/isc/util.h +++ b/lib/isc/include/isc/util.h @@ -285,12 +285,17 @@ extern void mock_assert(const int result, const char* const expression, #ifdef UNIT_TESTING #define RUNTIME_CHECK(expression) \ - mock_assert((int)(expression), #expression, __FILE__, __LINE__) + ((!(expression)) ? \ + (mock_assert(0, #expression, __FILE__, __LINE__), abort()) : (void)0) #else /* UNIT_TESTING */ +#ifndef CPPCHECK /*% Runtime Check */ #define RUNTIME_CHECK(cond) ISC_ERROR_RUNTIMECHECK(cond) +#else +#define RUNTIME_CHECK(e) if (!(e)) abort() +#endif #endif /* UNIT_TESTING */ From 6c2e138d7aac0e7487b2ba2df6796ba64ccfc26b Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 30 Jan 2020 17:27:08 +1100 Subject: [PATCH 02/13] simplify ISC_LIKELY/ISC_UNLIKELY for CPPCHECK --- lib/isc/include/isc/likely.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/isc/include/isc/likely.h b/lib/isc/include/isc/likely.h index 8cc5b2eb4b..6f65e1b27d 100644 --- a/lib/isc/include/isc/likely.h +++ b/lib/isc/include/isc/likely.h @@ -15,6 +15,10 @@ /*% * Performance */ +#ifdef CPPCHECK +#define ISC_LIKELY(x) (x) +#define ISC_UNLIKELY(x) (x) +#else #ifdef HAVE_BUILTIN_EXPECT #define ISC_LIKELY(x) __builtin_expect(!!(x), 1) #define ISC_UNLIKELY(x) __builtin_expect(!!(x), 0) @@ -22,5 +26,6 @@ #define ISC_LIKELY(x) (x) #define ISC_UNLIKELY(x) (x) #endif +#endif #endif /* ISC_LIKELY_H */ From 7b948c7335f6d2e5c761b190103a8cf18e3cf43d Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 30 Jan 2020 17:49:15 +1100 Subject: [PATCH 03/13] remove brackets --- lib/isc/pthreads/include/isc/mutex.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/isc/pthreads/include/isc/mutex.h b/lib/isc/pthreads/include/isc/mutex.h index 4f3a4d467e..4e98033aa6 100644 --- a/lib/isc/pthreads/include/isc/mutex.h +++ b/lib/isc/pthreads/include/isc/mutex.h @@ -103,10 +103,10 @@ void isc__mutex_init(isc_mutex_t *mp, const char *file, unsigned int line); #if ISC_MUTEX_PROFILE #define isc_mutex_destroy(mp) \ - (RUNTIME_CHECK(pthread_mutex_destroy((&(mp)->mutex)) == 0)) + RUNTIME_CHECK(pthread_mutex_destroy((&(mp)->mutex)) == 0) #else #define isc_mutex_destroy(mp) \ - (RUNTIME_CHECK(pthread_mutex_destroy((mp)) == 0)) + RUNTIME_CHECK(pthread_mutex_destroy((mp)) == 0) #endif #if ISC_MUTEX_PROFILE From f17b9b8dd141fa59717bee53fcd685d5e42eed5d Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 30 Jan 2020 17:57:25 +1100 Subject: [PATCH 04/13] make expression logical for cppcheck --- fuzz/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fuzz/main.c b/fuzz/main.c index 1e973e210c..ae409b38d1 100644 --- a/fuzz/main.c +++ b/fuzz/main.c @@ -97,7 +97,7 @@ int main(int argc, char **argv) UNUSED(argc); UNUSED(argv); - target = target ? target + 1 : argv[0]; + target = (target != NULL) ? target + 1 : argv[0]; if (strncmp(target, "lt-", 3) == 0) { target += 3; } From c65c06301c56860b5b75bbbc42c00f6459a646f8 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 30 Jan 2020 18:19:11 +1100 Subject: [PATCH 05/13] delay assignment until after REQUIRE --- lib/isc/timer.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/lib/isc/timer.c b/lib/isc/timer.c index f7027f5625..7e56d6df1e 100644 --- a/lib/isc/timer.c +++ b/lib/isc/timer.c @@ -239,7 +239,11 @@ isc_timer_create(isc_timermgr_t *manager0, isc_timertype_t type, isc_task_t *task, isc_taskaction_t action, void *arg, isc_timer_t **timerp) { - isc__timermgr_t *manager = (isc__timermgr_t *)manager0; + REQUIRE(VALID_MANAGER(manager0)); + REQUIRE(task != NULL); + REQUIRE(action != NULL); + + isc__timermgr_t *manager; isc__timer_t *timer; isc_result_t result; isc_time_t now; @@ -251,10 +255,7 @@ isc_timer_create(isc_timermgr_t *manager0, isc_timertype_t type, * called with 'arg' as the arg value. The new timer is returned * in 'timerp'. */ - - REQUIRE(VALID_MANAGER(manager)); - REQUIRE(task != NULL); - REQUIRE(action != NULL); + manager = (isc__timermgr_t *)manager0; if (expires == NULL) expires = isc_time_epoch; if (interval == NULL) @@ -352,7 +353,7 @@ isc_timer_reset(isc_timer_t *timer0, isc_timertype_t type, const isc_time_t *expires, const isc_interval_t *interval, bool purge) { - isc__timer_t *timer = (isc__timer_t *)timer0; + isc__timer_t *timer; isc_time_t now; isc__timermgr_t *manager; isc_result_t result; @@ -363,7 +364,8 @@ isc_timer_reset(isc_timer_t *timer0, isc_timertype_t type, * are purged from its task's event queue. */ - REQUIRE(VALID_TIMER(timer)); + REQUIRE(VALID_TIMER(timer0)); + timer = (isc__timer_t *)timer0; manager = timer->manager; REQUIRE(VALID_MANAGER(manager)); @@ -425,10 +427,11 @@ isc_timer_reset(isc_timer_t *timer0, isc_timertype_t type, isc_timertype_t isc_timer_gettype(isc_timer_t *timer0) { - isc__timer_t *timer = (isc__timer_t *)timer0; + isc__timer_t *timer; isc_timertype_t t; - REQUIRE(VALID_TIMER(timer)); + REQUIRE(VALID_TIMER(timer0)); + timer = (isc__timer_t *)timer0; LOCK(&timer->lock); t = timer->type; @@ -439,7 +442,7 @@ isc_timer_gettype(isc_timer_t *timer0) { isc_result_t isc_timer_touch(isc_timer_t *timer0) { - isc__timer_t *timer = (isc__timer_t *)timer0; + isc__timer_t *timer; isc_result_t result; isc_time_t now; @@ -447,7 +450,8 @@ isc_timer_touch(isc_timer_t *timer0) { * Set the last-touched time of 'timer' to the current time. */ - REQUIRE(VALID_TIMER(timer)); + REQUIRE(VALID_TIMER(timer0)); + timer = (isc__timer_t *)timer0; LOCK(&timer->lock); @@ -470,13 +474,14 @@ isc_timer_touch(isc_timer_t *timer0) { void isc_timer_attach(isc_timer_t *timer0, isc_timer_t **timerp) { - isc__timer_t *timer = (isc__timer_t *)timer0; + isc__timer_t *timer; /* * Attach *timerp to timer. */ - REQUIRE(VALID_TIMER(timer)); + REQUIRE(VALID_TIMER(timer0)); + timer = (isc__timer_t *)timer0; REQUIRE(timerp != NULL && *timerp == NULL); isc_refcount_increment(&timer->references); @@ -661,8 +666,8 @@ static void set_index(void *what, unsigned int index) { isc__timer_t *timer; + REQUIRE(VALID_TIMER(what)); timer = what; - REQUIRE(VALID_TIMER(timer)); timer->index = index; } @@ -707,9 +712,10 @@ isc_timermgr_create(isc_mem_t *mctx, isc_timermgr_t **managerp) { void isc_timermgr_poke(isc_timermgr_t *manager0) { - isc__timermgr_t *manager = (isc__timermgr_t *)manager0; + isc__timermgr_t *manager; - REQUIRE(VALID_MANAGER(manager)); + REQUIRE(VALID_MANAGER(manager0)); + manager = (isc__timermgr_t *)manager0; SIGNAL(&manager->wakeup); } From 704b9ee9d08c47740fe2401900b65f188ef07fe2 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 30 Jan 2020 18:55:36 +1100 Subject: [PATCH 06/13] skip if first is NULL --- lib/dns/zoneverify.c | 75 +++++++++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 28 deletions(-) diff --git a/lib/dns/zoneverify.c b/lib/dns/zoneverify.c index c19367b0a0..4f9264356d 100644 --- a/lib/dns/zoneverify.c +++ b/lib/dns/zoneverify.c @@ -1122,7 +1122,7 @@ free_element_heap(void *element, void *uap) { } static bool -checknext(const vctx_t *vctx, const struct nsec3_chain_fixed *first, +_checknext(const vctx_t *vctx, const struct nsec3_chain_fixed *first, const struct nsec3_chain_fixed *e) { char buf[512]; @@ -1162,6 +1162,37 @@ checknext(const vctx_t *vctx, const struct nsec3_chain_fixed *first, return (false); } +static inline bool +checknext(isc_mem_t *mctx, + const vctx_t *vctx, + const struct nsec3_chain_fixed *first, + struct nsec3_chain_fixed *prev, + const struct nsec3_chain_fixed *cur) +{ + bool result = _checknext(vctx, prev, cur); + + if (prev != first) { + free_element(mctx, prev); + } + + return (result); +} + +static inline bool +checklast(isc_mem_t *mctx, + const vctx_t *vctx, + struct nsec3_chain_fixed *first, + struct nsec3_chain_fixed *prev) +{ + bool result = _checknext(vctx, prev, first); + if (prev != first) { + free_element(mctx, prev); + } + free_element(mctx, first); + + return (result); +} + static isc_result_t verify_nsec3_chains(const vctx_t *vctx, isc_mem_t *mctx) { isc_result_t result = ISC_R_SUCCESS; @@ -1214,39 +1245,27 @@ verify_nsec3_chains(const vctx_t *vctx, isc_mem_t *mctx) { "not equal"); result = ISC_R_FAILURE; } - if (first == NULL || newchain(first, e)) { - if (prev != NULL) { - if (!checknext(vctx, prev, first)) { - result = ISC_R_FAILURE; - } - if (prev != first) { - free_element(mctx, prev); - } - } - if (first != NULL) { - free_element(mctx, first); - } + + if (first == NULL) { prev = first = e; - continue; + } else if (newchain(first, e)) { + if (!checklast(mctx, vctx, first, prev)) { + result = ISC_R_FAILURE; + } + + prev = first = e; + } else { + if (!checknext(mctx, vctx, first, prev, e)) { + result = ISC_R_FAILURE; + } + + prev = e; } - if (!checknext(vctx, prev, e)) { - result = ISC_R_FAILURE; - } - if (prev != first) { - free_element(mctx, prev); - } - prev = e; } if (prev != NULL) { - if (!checknext(vctx, prev, first)) { + if (!checklast(mctx, vctx, first, prev)) { result = ISC_R_FAILURE; } - if (prev != first) { - free_element(mctx, prev); - } - } - if (first != NULL) { - free_element(mctx, first); } do { if (f != NULL) { From d6de520bd15b0bf4114a1176240bbdde99002a6a Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 30 Jan 2020 19:38:47 +1100 Subject: [PATCH 07/13] delay assignment until after REQUIRE --- lib/dns/tests/rbt_serialize_test.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/dns/tests/rbt_serialize_test.c b/lib/dns/tests/rbt_serialize_test.c index 8a86d49985..edfa711a32 100644 --- a/lib/dns/tests/rbt_serialize_test.c +++ b/lib/dns/tests/rbt_serialize_test.c @@ -130,7 +130,7 @@ static isc_result_t write_data(FILE *file, unsigned char *datap, void *arg, uint64_t *crc) { isc_result_t result; size_t ret = 0; - data_holder_t *data = (data_holder_t *)datap; + data_holder_t *data; data_holder_t temp; off_t where; @@ -138,7 +138,8 @@ write_data(FILE *file, unsigned char *datap, void *arg, uint64_t *crc) { REQUIRE(file != NULL); REQUIRE(crc != NULL); - REQUIRE(data != NULL); + REQUIRE(datap != NULL); + data = (data_holder_t *)datap; REQUIRE((data->len == 0 && data->data == NULL) || (data->len != 0 && data->data != NULL)); From bb65e5729725930769e3ee420fec655efe37440c Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 30 Jan 2020 19:41:32 +1100 Subject: [PATCH 08/13] isc_mem_get cannot fail --- lib/dns/client.c | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/lib/dns/client.c b/lib/dns/client.c index 985b6f42c9..34e65bd066 100644 --- a/lib/dns/client.c +++ b/lib/dns/client.c @@ -1340,13 +1340,7 @@ dns_client_startresolve(dns_client_t *client, const dns_name_t *name, ISC_LIST_INIT(event->answerlist); rctx = isc_mem_get(mctx, sizeof(*rctx)); - if (rctx == NULL) - result = ISC_R_NOMEMORY; - else { - isc_mutex_init(&rctx->lock); - } - if (result != ISC_R_SUCCESS) - goto cleanup; + isc_mutex_init(&rctx->lock); result = getrdataset(mctx, &rdataset); if (result != ISC_R_SUCCESS) @@ -1748,13 +1742,7 @@ dns_client_startrequest(dns_client_t *client, dns_message_t *qmessage, action, arg, sizeof(*event)); ctx = isc_mem_get(client->mctx, sizeof(*ctx)); - if (ctx == NULL) - result = ISC_R_NOMEMORY; - else { - isc_mutex_init(&ctx->lock); - } - if (result != ISC_R_SUCCESS) - goto cleanup; + isc_mutex_init(&ctx->lock); ctx->client = client; ISC_LINK_INIT(ctx, link); @@ -1787,16 +1775,13 @@ dns_client_startrequest(dns_client_t *client, dns_message_t *qmessage, isc_refcount_decrement(&client->references); - cleanup: - if (ctx != NULL) { - LOCK(&client->lock); - ISC_LIST_UNLINK(client->reqctxs, ctx, link); - UNLOCK(&client->lock); - isc_mutex_destroy(&ctx->lock); - isc_mem_put(client->mctx, ctx, sizeof(*ctx)); - } - if (event != NULL) - isc_event_free(ISC_EVENT_PTR(&event)); + LOCK(&client->lock); + ISC_LIST_UNLINK(client->reqctxs, ctx, link); + UNLOCK(&client->lock); + isc_mutex_destroy(&ctx->lock); + isc_mem_put(client->mctx, ctx, sizeof(*ctx)); + + isc_event_free(ISC_EVENT_PTR(&event)); isc_task_detach(&tclone); dns_view_detach(&view); From 478e4ac201478e53bb40b70aa37ddc696c0c8872 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sat, 1 Feb 2020 17:59:19 +0100 Subject: [PATCH 09/13] Make the DbC checks to be consistent and cppcheck clean --- lib/isc/mem.c | 187 +++++++++++++++++++++++++------------------------- 1 file changed, 92 insertions(+), 95 deletions(-) diff --git a/lib/isc/mem.c b/lib/isc/mem.c index 9e0887782f..c65d9219d6 100644 --- a/lib/isc/mem.c +++ b/lib/isc/mem.c @@ -363,7 +363,6 @@ more_basic_blocks(isc__mem_t *ctx) { if (ctx->basic_table_count == ctx->basic_table_size) { table_size = ctx->basic_table_size + TABLE_INCREMENT; table = (ctx->memalloc)(table_size * sizeof(unsigned char *)); - RUNTIME_CHECK(table != NULL); ctx->malloced += table_size * sizeof(unsigned char *); if (ctx->malloced > ctx->maxmalloced) ctx->maxmalloced = ctx->malloced; @@ -380,7 +379,6 @@ more_basic_blocks(isc__mem_t *ctx) { } tmp = (ctx->memalloc)(NUM_BASIC_BLOCKS * ctx->mem_target); - RUNTIME_CHECK(tmp != NULL); ctx->total += NUM_BASIC_BLOCKS * ctx->mem_target;; ctx->basic_table[ctx->basic_table_count] = tmp; ctx->basic_table_count++; @@ -473,7 +471,6 @@ mem_getunlocked(isc__mem_t *ctx, size_t size) { * memget() was called on something beyond our upper limit. */ ret = (ctx->memalloc)(size); - RUNTIME_CHECK(ret != NULL); ctx->total += size; ctx->inuse += size; ctx->stats[ctx->max_size].gets++; @@ -596,7 +593,6 @@ mem_get(isc__mem_t *ctx, size_t size) { size += 1; #endif ret = (ctx->memalloc)(size); - RUNTIME_CHECK(ret != NULL); if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0)) { if (ISC_LIKELY(ret != NULL)) @@ -723,10 +719,10 @@ initialize_action(void) { static void mem_create(isc_mem_t **ctxp, unsigned int flags) { - isc__mem_t *ctx; - REQUIRE(ctxp != NULL && *ctxp == NULL); + isc__mem_t *ctx; + STATIC_ASSERT((ALIGNMENT_SIZE & (ALIGNMENT_SIZE - 1)) == 0, "wrong alignment size"); @@ -774,7 +770,6 @@ mem_create(isc_mem_t **ctxp, unsigned int flags) ctx->highest = NULL; ctx->stats = (ctx->memalloc)((ctx->max_size+1) * sizeof(struct stats)); - RUNTIME_CHECK(ctx->stats != NULL); memset(ctx->stats, 0, (ctx->max_size + 1) * sizeof(struct stats)); ctx->malloced += (ctx->max_size+1) * sizeof(struct stats); @@ -784,7 +779,6 @@ mem_create(isc_mem_t **ctxp, unsigned int flags) ctx->mem_target = DEF_MEM_TARGET; ctx->freelists = (ctx->memalloc)(ctx->max_size * sizeof(element *)); - RUNTIME_CHECK(ctx->freelists != NULL); memset(ctx->freelists, 0, ctx->max_size * sizeof(element *)); ctx->malloced += ctx->max_size * sizeof(element *); @@ -797,7 +791,6 @@ mem_create(isc_mem_t **ctxp, unsigned int flags) ctx->debuglist = (ctx->memalloc)((DEBUG_TABLE_COUNT * sizeof(debuglist_t))); - RUNTIME_CHECK(ctx->debuglist != NULL); for (i = 0; i < DEBUG_TABLE_COUNT; i++) ISC_LIST_INIT(ctx->debuglist[i]); ctx->malloced += DEBUG_TABLE_COUNT * sizeof(debuglist_t); @@ -895,11 +888,11 @@ destroy(isc__mem_t *ctx) { void isc_mem_attach(isc_mem_t *source0, isc_mem_t **targetp) { - isc__mem_t *source = (isc__mem_t *)source0; - - REQUIRE(VALID_CONTEXT(source)); + REQUIRE(VALID_CONTEXT(source0)); REQUIRE(targetp != NULL && *targetp == NULL); + isc__mem_t *source = (isc__mem_t *)source0; + isc_refcount_increment(&source->references); *targetp = (isc_mem_t *)source; @@ -908,6 +901,7 @@ isc_mem_attach(isc_mem_t *source0, isc_mem_t **targetp) { void isc_mem_detach(isc_mem_t **ctxp) { REQUIRE(ctxp != NULL && VALID_CONTEXT(*ctxp)); + isc__mem_t *ctx = (isc__mem_t *)*ctxp; *ctxp = NULL; @@ -931,6 +925,7 @@ void isc___mem_putanddetach(isc_mem_t **ctxp, void *ptr, size_t size FLARG) { REQUIRE(ctxp != NULL && VALID_CONTEXT(*ctxp)); REQUIRE(ptr != NULL); + isc__mem_t *ctx = (isc__mem_t *)*ctxp; *ctxp = NULL; @@ -970,16 +965,14 @@ destroy: void isc_mem_destroy(isc_mem_t **ctxp) { - isc__mem_t *ctx; - /* * This routine provides legacy support for callers who use mctxs * without attaching/detaching. */ - REQUIRE(ctxp != NULL); - ctx = (isc__mem_t *)*ctxp; - REQUIRE(VALID_CONTEXT(ctx)); + REQUIRE(ctxp != NULL && VALID_CONTEXT(*ctxp)); + + isc__mem_t *ctx = (isc__mem_t *)*ctxp; #if ISC_MEM_TRACKLINES if (isc_refcount_decrement(&ctx->references) > 1) { @@ -996,12 +989,12 @@ isc_mem_destroy(isc_mem_t **ctxp) { void * isc___mem_get(isc_mem_t *ctx0, size_t size FLARG) { + REQUIRE(VALID_CONTEXT(ctx0)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; void *ptr; bool call_water = false; - REQUIRE(VALID_CONTEXT(ctx)); - if (ISC_UNLIKELY((isc_mem_debugging & (ISC_MEM_DEBUGSIZE|ISC_MEM_DEBUGCTX)) != 0)) return (isc__mem_allocate(ctx0, size FLARG_PASS)); @@ -1040,14 +1033,14 @@ isc___mem_get(isc_mem_t *ctx0, size_t size FLARG) { void isc___mem_put(isc_mem_t *ctx0, void *ptr, size_t size FLARG) { + REQUIRE(VALID_CONTEXT(ctx0)); + REQUIRE(ptr != NULL); + isc__mem_t *ctx = (isc__mem_t *)ctx0; bool call_water = false; size_info *si; size_t oldsize; - REQUIRE(VALID_CONTEXT(ctx)); - REQUIRE(ptr != NULL); - if (ISC_UNLIKELY((isc_mem_debugging & (ISC_MEM_DEBUGSIZE|ISC_MEM_DEBUGCTX)) != 0)) { @@ -1092,9 +1085,9 @@ isc___mem_put(isc_mem_t *ctx0, void *ptr, size_t size FLARG) { void isc_mem_waterack(isc_mem_t *ctx0, int flag) { - isc__mem_t *ctx = (isc__mem_t *)ctx0; + REQUIRE(VALID_CONTEXT(ctx0)); - REQUIRE(VALID_CONTEXT(ctx)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; MCTXLOCK(ctx); if (flag == ISC_MEM_LOWATER) @@ -1144,12 +1137,13 @@ print_active(isc__mem_t *mctx, FILE *out) { */ void isc_mem_stats(isc_mem_t *ctx0, FILE *out) { + REQUIRE(VALID_CONTEXT(ctx0)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; size_t i; const struct stats *s; const isc__mempool_t *pool; - REQUIRE(VALID_CONTEXT(ctx)); MCTXLOCK(ctx); for (i = 0; i <= ctx->max_size; i++) { @@ -1232,12 +1226,12 @@ mem_allocateunlocked(isc_mem_t *ctx0, size_t size) { void * isc___mem_allocate(isc_mem_t *ctx0, size_t size FLARG) { + REQUIRE(VALID_CONTEXT(ctx0)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; size_info *si; bool call_water = false; - REQUIRE(VALID_CONTEXT(ctx)); - MCTXLOCK(ctx); si = mem_allocateunlocked((isc_mem_t *)ctx, size); if (((ctx->flags & ISC_MEMFLAG_INTERNAL) == 0)) { @@ -1274,12 +1268,11 @@ isc___mem_allocate(isc_mem_t *ctx0, size_t size FLARG) { void * isc___mem_reallocate(isc_mem_t *ctx0, void *ptr, size_t size FLARG) { - isc__mem_t *ctx = (isc__mem_t *)ctx0; + REQUIRE(VALID_CONTEXT(ctx0)); + void *new_ptr = NULL; size_t oldsize, copysize; - REQUIRE(VALID_CONTEXT(ctx)); - /* * This function emulates the realloc(3) standard library function: * - if size > 0, allocate new memory; and if ptr is non NULL, copy @@ -1315,14 +1308,14 @@ isc___mem_reallocate(isc_mem_t *ctx0, void *ptr, size_t size FLARG) { void isc___mem_free(isc_mem_t *ctx0, void *ptr FLARG) { + REQUIRE(VALID_CONTEXT(ctx0)); + REQUIRE(ptr != NULL); + isc__mem_t *ctx = (isc__mem_t *)ctx0; size_info *si; size_t size; bool call_water= false; - REQUIRE(VALID_CONTEXT(ctx)); - REQUIRE(ptr != NULL); - if (ISC_UNLIKELY((isc_mem_debugging & ISC_MEM_DEBUGCTX) != 0)) { si = &(((size_info *)ptr)[-2]); REQUIRE(si->u.ctx == ctx); @@ -1373,13 +1366,13 @@ isc___mem_free(isc_mem_t *ctx0, void *ptr FLARG) { char * isc___mem_strdup(isc_mem_t *mctx0, const char *s FLARG) { + REQUIRE(VALID_CONTEXT(mctx0)); + REQUIRE(s != NULL); + isc__mem_t *mctx = (isc__mem_t *)mctx0; size_t len; char *ns; - REQUIRE(VALID_CONTEXT(mctx)); - REQUIRE(s != NULL); - len = strlen(s) + 1; ns = isc__mem_allocate((isc_mem_t *)mctx, len FLARG_PASS); @@ -1392,9 +1385,10 @@ isc___mem_strdup(isc_mem_t *mctx0, const char *s FLARG) { void isc_mem_setdestroycheck(isc_mem_t *ctx0, bool flag) { + REQUIRE(VALID_CONTEXT(ctx0)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; - REQUIRE(VALID_CONTEXT(ctx)); MCTXLOCK(ctx); ctx->checkfree = flag; @@ -1404,10 +1398,11 @@ isc_mem_setdestroycheck(isc_mem_t *ctx0, bool flag) { size_t isc_mem_inuse(isc_mem_t *ctx0) { + REQUIRE(VALID_CONTEXT(ctx0)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; size_t inuse; - REQUIRE(VALID_CONTEXT(ctx)); MCTXLOCK(ctx); inuse = ctx->inuse; @@ -1419,10 +1414,11 @@ isc_mem_inuse(isc_mem_t *ctx0) { size_t isc_mem_maxinuse(isc_mem_t *ctx0) { + REQUIRE(VALID_CONTEXT(ctx0)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; size_t maxinuse; - REQUIRE(VALID_CONTEXT(ctx)); MCTXLOCK(ctx); maxinuse = ctx->maxinuse; @@ -1434,10 +1430,11 @@ isc_mem_maxinuse(isc_mem_t *ctx0) { size_t isc_mem_total(isc_mem_t *ctx0) { + REQUIRE(VALID_CONTEXT(ctx0)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; size_t total; - REQUIRE(VALID_CONTEXT(ctx)); MCTXLOCK(ctx); total = ctx->total; @@ -1451,14 +1448,14 @@ void isc_mem_setwater(isc_mem_t *ctx0, isc_mem_water_t water, void *water_arg, size_t hiwater, size_t lowater) { + REQUIRE(VALID_CONTEXT(ctx0)); + REQUIRE(hiwater >= lowater); + isc__mem_t *ctx = (isc__mem_t *)ctx0; bool callwater = false; isc_mem_water_t oldwater; void *oldwater_arg; - REQUIRE(VALID_CONTEXT(ctx)); - REQUIRE(hiwater >= lowater); - MCTXLOCK(ctx); oldwater = ctx->water; oldwater_arg = ctx->water_arg; @@ -1486,9 +1483,9 @@ isc_mem_setwater(isc_mem_t *ctx0, isc_mem_water_t water, void *water_arg, bool isc_mem_isovermem(isc_mem_t *ctx0) { - isc__mem_t *ctx = (isc__mem_t *)ctx0; + REQUIRE(VALID_CONTEXT(ctx0)); - REQUIRE(VALID_CONTEXT(ctx)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; /* * We don't bother to lock the context because 100% accuracy isn't @@ -1500,9 +1497,9 @@ isc_mem_isovermem(isc_mem_t *ctx0) { void isc_mem_setname(isc_mem_t *ctx0, const char *name, void *tag) { - isc__mem_t *ctx = (isc__mem_t *)ctx0; + REQUIRE(VALID_CONTEXT(ctx0)); - REQUIRE(VALID_CONTEXT(ctx)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; LOCK(&ctx->lock); strlcpy(ctx->name, name, sizeof(ctx->name)); @@ -1512,9 +1509,9 @@ isc_mem_setname(isc_mem_t *ctx0, const char *name, void *tag) { const char * isc_mem_getname(isc_mem_t *ctx0) { - isc__mem_t *ctx = (isc__mem_t *)ctx0; + REQUIRE(VALID_CONTEXT(ctx0)); - REQUIRE(VALID_CONTEXT(ctx)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; if (ctx->name[0] == 0) return (""); @@ -1524,9 +1521,9 @@ isc_mem_getname(isc_mem_t *ctx0) { void * isc_mem_gettag(isc_mem_t *ctx0) { - isc__mem_t *ctx = (isc__mem_t *)ctx0; + REQUIRE(VALID_CONTEXT(ctx0)); - REQUIRE(VALID_CONTEXT(ctx)); + isc__mem_t *ctx = (isc__mem_t *)ctx0; return (ctx->tag); } @@ -1537,19 +1534,18 @@ isc_mem_gettag(isc_mem_t *ctx0) { void isc_mempool_create(isc_mem_t *mctx0, size_t size, isc_mempool_t **mpctxp) { - isc__mem_t *mctx = (isc__mem_t *)mctx0; - isc__mempool_t *mpctx; - - REQUIRE(VALID_CONTEXT(mctx)); + REQUIRE(VALID_CONTEXT(mctx0)); REQUIRE(size > 0U); REQUIRE(mpctxp != NULL && *mpctxp == NULL); + isc__mem_t *mctx = (isc__mem_t *)mctx0; + isc__mempool_t *mpctx; + /* * Allocate space for this pool, initialize values, and if all works * well, attach to the memory context. */ mpctx = isc_mem_get((isc_mem_t *)mctx, sizeof(isc__mempool_t)); - RUNTIME_CHECK(mpctx != NULL); mpctx->common.impmagic = MEMPOOL_MAGIC; mpctx->common.magic = ISCAPI_MPOOL_MAGIC; @@ -1583,10 +1579,10 @@ isc_mempool_create(isc_mem_t *mctx0, size_t size, isc_mempool_t **mpctxp) { void isc_mempool_setname(isc_mempool_t *mpctx0, const char *name) { - isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; - + REQUIRE(VALID_MEMPOOL(mpctx0)); REQUIRE(name != NULL); - REQUIRE(VALID_MEMPOOL(mpctx)); + + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; #if ISC_MEMPOOL_NAMES if (mpctx->lock != NULL) @@ -1604,14 +1600,15 @@ isc_mempool_setname(isc_mempool_t *mpctx0, const char *name) { void isc_mempool_destroy(isc_mempool_t **mpctxp) { + REQUIRE(mpctxp != NULL); + REQUIRE(VALID_MEMPOOL(*mpctxp)); + isc__mempool_t *mpctx; isc__mem_t *mctx; isc_mutex_t *lock; element *item; - REQUIRE(mpctxp != NULL); mpctx = (isc__mempool_t *)*mpctxp; - REQUIRE(VALID_MEMPOOL(mpctx)); #if ISC_MEMPOOL_NAMES if (mpctx->allocated > 0) UNEXPECTED_ERROR(__FILE__, __LINE__, @@ -1668,24 +1665,25 @@ isc_mempool_destroy(isc_mempool_t **mpctxp) { void isc_mempool_associatelock(isc_mempool_t *mpctx0, isc_mutex_t *lock) { + REQUIRE(VALID_MEMPOOL(mpctx0)); + REQUIRE(lock != NULL); + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; - REQUIRE(VALID_MEMPOOL(mpctx)); REQUIRE(mpctx->lock == NULL); - REQUIRE(lock != NULL); mpctx->lock = lock; } void * isc__mempool_get(isc_mempool_t *mpctx0 FLARG) { + REQUIRE(VALID_MEMPOOL(mpctx0)); + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; element *item; isc__mem_t *mctx; unsigned int i; - REQUIRE(VALID_MEMPOOL(mpctx)); - mctx = mpctx->mctx; if (mpctx->lock != NULL) @@ -1755,14 +1753,12 @@ isc__mempool_get(isc_mempool_t *mpctx0 FLARG) { /* coverity[+free : arg-1] */ void isc__mempool_put(isc_mempool_t *mpctx0, void *mem FLARG) { - isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; - isc__mem_t *mctx; - element *item; - - REQUIRE(VALID_MEMPOOL(mpctx)); + REQUIRE(VALID_MEMPOOL(mpctx0)); REQUIRE(mem != NULL); - mctx = mpctx->mctx; + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; + isc__mem_t *mctx = mpctx->mctx; + element *item; if (mpctx->lock != NULL) LOCK(mpctx->lock); @@ -1813,9 +1809,9 @@ isc__mempool_put(isc_mempool_t *mpctx0, void *mem FLARG) { void isc_mempool_setfreemax(isc_mempool_t *mpctx0, unsigned int limit) { - isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; + REQUIRE(VALID_MEMPOOL(mpctx0)); - REQUIRE(VALID_MEMPOOL(mpctx)); + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; if (mpctx->lock != NULL) LOCK(mpctx->lock); @@ -1828,11 +1824,11 @@ isc_mempool_setfreemax(isc_mempool_t *mpctx0, unsigned int limit) { unsigned int isc_mempool_getfreemax(isc_mempool_t *mpctx0) { + REQUIRE(VALID_MEMPOOL(mpctx0)); + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; unsigned int freemax; - REQUIRE(VALID_MEMPOOL(mpctx)); - if (mpctx->lock != NULL) LOCK(mpctx->lock); @@ -1846,10 +1842,11 @@ isc_mempool_getfreemax(isc_mempool_t *mpctx0) { unsigned int isc_mempool_getfreecount(isc_mempool_t *mpctx0) { + REQUIRE(VALID_MEMPOOL(mpctx0)); + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; unsigned int freecount; - REQUIRE(VALID_MEMPOOL(mpctx)); if (mpctx->lock != NULL) LOCK(mpctx->lock); @@ -1864,11 +1861,10 @@ isc_mempool_getfreecount(isc_mempool_t *mpctx0) { void isc_mempool_setmaxalloc(isc_mempool_t *mpctx0, unsigned int limit) { - isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; - + REQUIRE(VALID_MEMPOOL(mpctx0)); REQUIRE(limit > 0); - REQUIRE(VALID_MEMPOOL(mpctx)); + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; if (mpctx->lock != NULL) LOCK(mpctx->lock); @@ -1881,11 +1877,11 @@ isc_mempool_setmaxalloc(isc_mempool_t *mpctx0, unsigned int limit) { unsigned int isc_mempool_getmaxalloc(isc_mempool_t *mpctx0) { + REQUIRE(VALID_MEMPOOL(mpctx0)); + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; unsigned int maxalloc; - REQUIRE(VALID_MEMPOOL(mpctx)); - if (mpctx->lock != NULL) LOCK(mpctx->lock); @@ -1899,10 +1895,11 @@ isc_mempool_getmaxalloc(isc_mempool_t *mpctx0) { unsigned int isc_mempool_getallocated(isc_mempool_t *mpctx0) { + REQUIRE(VALID_MEMPOOL(mpctx0)); + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; unsigned int allocated; - REQUIRE(VALID_MEMPOOL(mpctx)); if (mpctx->lock != NULL) LOCK(mpctx->lock); @@ -1917,10 +1914,10 @@ isc_mempool_getallocated(isc_mempool_t *mpctx0) { void isc_mempool_setfillcount(isc_mempool_t *mpctx0, unsigned int limit) { - isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; - + REQUIRE(VALID_MEMPOOL(mpctx0)); REQUIRE(limit > 0); - REQUIRE(VALID_MEMPOOL(mpctx)); + + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; if (mpctx->lock != NULL) LOCK(mpctx->lock); @@ -1933,12 +1930,12 @@ isc_mempool_setfillcount(isc_mempool_t *mpctx0, unsigned int limit) { unsigned int isc_mempool_getfillcount(isc_mempool_t *mpctx0) { + REQUIRE(VALID_MEMPOOL(mpctx0)); + isc__mempool_t *mpctx = (isc__mempool_t *)mpctx0; unsigned int fillcount; - REQUIRE(VALID_MEMPOOL(mpctx)); - if (mpctx->lock != NULL) LOCK(mpctx->lock); @@ -2011,10 +2008,10 @@ static int xml_renderctx(isc__mem_t *ctx, summarystat_t *summary, xmlTextWriterPtr writer) { - int xmlrc; - REQUIRE(VALID_CONTEXT(ctx)); + int xmlrc; + MCTXLOCK(ctx); TRY0(xmlTextWriterStartElement(writer, ISC_XMLCHAR "context")); @@ -2196,13 +2193,13 @@ isc_mem_renderxml(void *writer0) { static isc_result_t json_renderctx(isc__mem_t *ctx, summarystat_t *summary, json_object *array) { - json_object *ctxobj, *obj; - char buf[1024]; - REQUIRE(VALID_CONTEXT(ctx)); REQUIRE(summary != NULL); REQUIRE(array != NULL); + json_object *ctxobj, *obj; + char buf[1024]; + MCTXLOCK(ctx); summary->contextsize += sizeof(*ctx) + @@ -2409,11 +2406,11 @@ isc__mem_free(isc_mem_t *mctx, void *ptr FLARG) { void isc__mem_printactive(isc_mem_t *ctx0, FILE *file) { #if ISC_MEM_TRACKLINES - isc__mem_t *ctx = (isc__mem_t *)ctx0; - - REQUIRE(VALID_CONTEXT(ctx)); + REQUIRE(VALID_CONTEXT(ctx0)); REQUIRE(file != NULL); + isc__mem_t *ctx = (isc__mem_t *)ctx0; + print_active(ctx, file); #else UNUSED(ctx0); From 05ae2e48ab1bd4477a4acec11a18c2fdd1694cb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sat, 1 Feb 2020 18:24:43 +0100 Subject: [PATCH 10/13] Change pk11_mem_get() so it cannot soft-fail --- lib/isc/pk11.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/isc/pk11.c b/lib/isc/pk11.c index 2002c7bb81..1fc3e8c538 100644 --- a/lib/isc/pk11.c +++ b/lib/isc/pk11.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -21,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -151,8 +153,12 @@ pk11_mem_get(size_t size) { ptr = isc_mem_get(pk11_mctx, size); else { ptr = malloc(size); - if (ptr != NULL) - allocsize += (int)size; + if (ptr == NULL && size != 0) { + char strbuf[ISC_STRERRORSIZE]; + strerror_r(errno, strbuf, sizeof(strbuf)); + isc_error_fatal(__FILE__, __LINE__, "malloc failed: %s", + strbuf); + } } UNLOCK(&alloclock); @@ -323,8 +329,6 @@ pk11_get_session(pk11_context_t *ctx, pk11_optype_t optype, UNLOCK(&sessionlock); sp = pk11_mem_get(sizeof(*sp)); - if (sp == NULL) - return (ISC_R_NOMEMORY); sp->magic = SES_MAGIC; sp->token = token; sp->session = CK_INVALID_HANDLE; @@ -479,7 +483,6 @@ scan_slots(void) { if (slotCount == 0) return; slotList = pk11_mem_get(sizeof(CK_SLOT_ID) * slotCount); - RUNTIME_CHECK(slotList != NULL); PK11_FATALCHECK(pkcs_C_GetSlotList, (CK_FALSE, slotList, &slotCount)); for (i = 0; i < slotCount; i++) { @@ -490,7 +493,6 @@ scan_slots(void) { if (rv != CKR_OK) continue; token = pk11_mem_get(sizeof(*token)); - RUNTIME_CHECK(token != NULL); token->magic = TOK_MAGIC; token->slotid = slot; ISC_LINK_INIT(token, link); From c00def343f2bd96d9d87b37b53dc7e98a8c91b65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sat, 1 Feb 2020 19:35:36 +0100 Subject: [PATCH 11/13] Suppress cppcheck false positive nullPointerArithmeticRedundantCheck --- lib/dns/tests/rbt_serialize_test.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/dns/tests/rbt_serialize_test.c b/lib/dns/tests/rbt_serialize_test.c index edfa711a32..188cb31d05 100644 --- a/lib/dns/tests/rbt_serialize_test.c +++ b/lib/dns/tests/rbt_serialize_test.c @@ -403,7 +403,9 @@ deserialize_corrupt_test(void **state) { close(fd); /* Randomly fuzz a portion of the memory */ + /* cppcheck-suppress nullPointerArithmeticRedundantCheck */ p = base + (isc_random_uniform(filesize)); + /* cppcheck-suppress nullPointerArithmeticRedundantCheck */ q = base + filesize; q -= (isc_random_uniform(q - p)); while (p++ < q) { From 2868eafc4612deaa6f45f79afc425f05063ecb04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sat, 1 Feb 2020 19:37:54 +0100 Subject: [PATCH 12/13] Suppress unknownMacro directive which is currently broken with OpenSSL --- util/suppressions.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/util/suppressions.txt b/util/suppressions.txt index dbfa38b4af..ea5ff3b292 100644 --- a/util/suppressions.txt +++ b/util/suppressions.txt @@ -1,2 +1,3 @@ unmatchedSuppression:* preprocessorErrorDirective:* +unknownMacro:* From b8be29fee61c46170dc087ee72b88a84ef9f8d21 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 4 Feb 2020 11:00:58 +0100 Subject: [PATCH 13/13] Add a note on memory allocation isc__memalloc_t must deal with memory allocation failure and must never return NULL. --- lib/isc/mem.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/isc/mem.c b/lib/isc/mem.c index c65d9219d6..30399bf81f 100644 --- a/lib/isc/mem.c +++ b/lib/isc/mem.c @@ -126,6 +126,11 @@ static isc_mutex_t contextslock; */ static uint64_t totallost; +/*% + * Memory allocation and free function definitions. + * isc__memalloc_t must deal with memory allocation failure + * and must never return NULL. + */ typedef void * (*isc__memalloc_t)(size_t); typedef void (*isc__memfree_t)(void *);