From 1bf451cffba5da84166dda48ec957e0b9cf45bee Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 16 May 2019 15:03:54 -0400 Subject: [PATCH 1/4] rng_test_helpers: add a needless lock/unlock pair to please coverity Fix for CID 1444908 --- src/test/rng_test_helpers.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/rng_test_helpers.c b/src/test/rng_test_helpers.c index 262d380bda..04b7138471 100644 --- a/src/test/rng_test_helpers.c +++ b/src/test/rng_test_helpers.c @@ -183,11 +183,14 @@ void testing_enable_prefilled_rng(const void *buffer, size_t buflen) { tor_assert(buflen > 0); + tor_assert(!rng_mutex); rng_mutex = tor_mutex_new(); + tor_mutex_acquire(rng_mutex); prefilled_rng_buffer = tor_memdup(buffer, buflen); prefilled_rng_buflen = buflen; prefilled_rng_idx = 0; + tor_mutex_release(rng_mutex); MOCK(crypto_rand, crypto_rand_prefilled); MOCK(crypto_strongest_rand_, mock_crypto_strongest_rand); From 0a9685b3a7437e8851f8cb65fea3d0a16b7833a7 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 16 May 2019 15:21:18 -0400 Subject: [PATCH 2/4] hs tests: explicitly free 'service' variable. This should fix about 15 CID issues, where coverity can't tell that hs_free_all() frees the service we allocated. --- src/test/test_hs_common.c | 4 ++++ src/test/test_hs_service.c | 48 +++++++++++++++++++++++++++++++++----- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c index bb41f1f870..abded6021e 100644 --- a/src/test/test_hs_common.c +++ b/src/test/test_hs_common.c @@ -603,6 +603,10 @@ test_desc_reupload_logic(void *arg) SMARTLIST_FOREACH(ns->routerstatus_list, routerstatus_t *, rs, routerstatus_free(rs)); smartlist_clear(ns->routerstatus_list); + if (service) { + remove_service(get_hs_service_map(), service); + hs_service_free(service); + } networkstatus_vote_free(ns); cleanup_nodelist(); hs_free_all(); diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index c4a8583696..a303f10411 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -878,6 +878,10 @@ test_helper_functions(void *arg) done: /* This will free the service and all objects associated to it. */ + if (service) { + remove_service(get_hs_service_map(), service); + hs_service_free(service); + } hs_service_free_all(); UNMOCK(node_get_by_id); } @@ -887,7 +891,7 @@ static void test_intro_circuit_opened(void *arg) { int flags = CIRCLAUNCH_NEED_UPTIME | CIRCLAUNCH_IS_INTERNAL; - hs_service_t *service; + hs_service_t *service = NULL; origin_circuit_t *circ = NULL; (void) arg; @@ -935,6 +939,10 @@ test_intro_circuit_opened(void *arg) done: circuit_free_(TO_CIRCUIT(circ)); + if (service) { + remove_service(get_hs_service_map(), service); + hs_service_free(service); + } hs_free_all(); UNMOCK(circuit_mark_for_close_); UNMOCK(relay_send_command_from_edge_); @@ -949,7 +957,7 @@ test_intro_established(void *arg) int flags = CIRCLAUNCH_NEED_UPTIME | CIRCLAUNCH_IS_INTERNAL; uint8_t payload[RELAY_PAYLOAD_SIZE] = {0}; origin_circuit_t *circ = NULL; - hs_service_t *service; + hs_service_t *service = NULL; hs_service_intro_point_t *ip = NULL; (void) arg; @@ -1010,6 +1018,10 @@ test_intro_established(void *arg) done: if (circ) circuit_free_(TO_CIRCUIT(circ)); + if (service) { + remove_service(get_hs_service_map(), service); + hs_service_free(service); + } hs_free_all(); UNMOCK(circuit_mark_for_close_); } @@ -1021,7 +1033,7 @@ test_rdv_circuit_opened(void *arg) { int flags = CIRCLAUNCH_NEED_UPTIME | CIRCLAUNCH_IS_INTERNAL; origin_circuit_t *circ = NULL; - hs_service_t *service; + hs_service_t *service = NULL; (void) arg; @@ -1052,6 +1064,10 @@ test_rdv_circuit_opened(void *arg) done: circuit_free_(TO_CIRCUIT(circ)); + if (service) { + remove_service(get_hs_service_map(), service); + hs_service_free(service); + } hs_free_all(); UNMOCK(circuit_mark_for_close_); UNMOCK(relay_send_command_from_edge_); @@ -1139,6 +1155,10 @@ test_closing_intro_circs(void *arg) circuit_free_(TO_CIRCUIT(intro_circ)); } /* Frees the service object. */ + if (service) { + remove_service(get_hs_service_map(), service); + hs_service_free(service); + } hs_free_all(); UNMOCK(assert_circuit_ok); } @@ -1151,7 +1171,7 @@ test_introduce2(void *arg) int flags = CIRCLAUNCH_NEED_UPTIME | CIRCLAUNCH_IS_INTERNAL; uint8_t payload[RELAY_PAYLOAD_SIZE] = {0}; origin_circuit_t *circ = NULL; - hs_service_t *service; + hs_service_t *service = NULL; hs_service_intro_point_t *ip = NULL; (void) arg; @@ -1218,6 +1238,10 @@ test_introduce2(void *arg) dummy_state = NULL; if (circ) circuit_free_(TO_CIRCUIT(circ)); + if (service) { + remove_service(get_hs_service_map(), service); + hs_service_free(service); + } hs_free_all(); UNMOCK(circuit_mark_for_close_); } @@ -1302,6 +1326,10 @@ test_service_event(void *arg) done: hs_circuitmap_remove_circuit(TO_CIRCUIT(circ)); circuit_free_(TO_CIRCUIT(circ)); + if (service) { + remove_service(get_hs_service_map(), service); + hs_service_free(service); + } hs_free_all(); UNMOCK(circuit_mark_for_close_); } @@ -1312,7 +1340,7 @@ test_rotate_descriptors(void *arg) { int ret; time_t next_rotation_time, now; - hs_service_t *service; + hs_service_t *service = NULL; hs_service_descriptor_t *desc_next; (void) arg; @@ -1404,6 +1432,10 @@ test_rotate_descriptors(void *arg) tt_assert(service->desc_next); done: + if (service) { + remove_service(get_hs_service_map(), service); + hs_service_free(service); + } hs_free_all(); UNMOCK(get_or_state); UNMOCK(circuit_mark_for_close_); @@ -1417,7 +1449,7 @@ test_build_update_descriptors(void *arg) { int ret; node_t *node; - hs_service_t *service; + hs_service_t *service = NULL; hs_service_intro_point_t *ip_cur, *ip_next; routerinfo_t ri; @@ -1634,6 +1666,10 @@ test_build_update_descriptors(void *arg) tt_u64_op(service->desc_next->next_upload_time, OP_EQ, 0); done: + if (service) { + remove_service(get_hs_service_map(), service); + hs_service_free(service); + } hs_free_all(); nodelist_free_all(); } From d5db40a0145457f6a1ee24ac1310511eaf62b959 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 16 May 2019 15:34:28 -0400 Subject: [PATCH 3/4] test_channel_listener: free 'chan' explicitly This should fix CID 1437442, where coverity can't tell that channel_free_all() frees the fake channel we allocated. --- src/test/test_channel.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 4c2bbc86b4..6a6bc9d810 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -1540,6 +1540,10 @@ test_channel_listener(void *arg) channel_listener_dump_statistics(chan, LOG_INFO); done: + if (chan) { + channel_listener_unregister(chan); + tor_free(chan); + } channel_free_all(); } @@ -1566,4 +1570,3 @@ struct testcase_t channel_tests[] = { NULL, NULL }, END_OF_TESTCASES }; - From 6246f4539ed317e05b773b0e373f1aa369d3759d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 16 May 2019 15:38:08 -0400 Subject: [PATCH 4/4] changes file for coverity fixes in tests (30150) --- changes/ticket30150 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/ticket30150 diff --git a/changes/ticket30150 b/changes/ticket30150 new file mode 100644 index 0000000000..70808fa5db --- /dev/null +++ b/changes/ticket30150 @@ -0,0 +1,4 @@ + o Minor bugfixes (static analysis): + - Fix several spurious Coverity warnings about the unit tests, to lower our + chances of missing any real warnings in the future. Fixes bug 30150; + bugfix on 0.3.5.1-alpha and various other Tor versions.