From 7c96d1b35aa4cad49f3f961fd2615b196908b786 Mon Sep 17 00:00:00 2001 From: Alexander Bushnev Date: Fri, 22 Nov 2024 23:11:28 +0100 Subject: [PATCH 1/4] Add AddressSanitizer to CI tests --- .github/workflows/build-check.yaml | 4 ++-- CMakeLists.txt | 8 +++++++- GNUmakefile | 6 +++++- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/.github/workflows/build-check.yaml b/.github/workflows/build-check.yaml index be3e52d7e..4f73f6d81 100644 --- a/.github/workflows/build-check.yaml +++ b/.github/workflows/build-check.yaml @@ -30,7 +30,7 @@ jobs: - name: Build & run tests run: | sudo apt install -y ninja-build - CMAKE_GENERATOR=Ninja make test + CMAKE_GENERATOR=Ninja ASAN=ON make test check_format: name: Check codebase format with clang-format @@ -290,6 +290,6 @@ jobs: - name: Build & test pico run: | sudo apt install -y ninja-build - CMAKE_GENERATOR=Ninja make + CMAKE_GENERATOR=Ninja ASAN=ON make python3 ./build/tests/no_router.py timeout-minutes: 5 diff --git a/CMakeLists.txt b/CMakeLists.txt index 9ef984042..7a9fb6d10 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -97,7 +97,6 @@ else() add_compile_options(-Wpedantic) endif() # add_compile_options(-Wconversion) - # add_link_options(-fsanitize=address) elseif(MSVC) add_compile_options(/W4 /WX /Od /wd4127) elseif(CMAKE_SYSTEM_NAME MATCHES "Generic") @@ -379,12 +378,14 @@ option(BUILD_EXAMPLES "Use this to also build the examples." ON) option(BUILD_TOOLS "Use this to also build the tools." OFF) option(BUILD_TESTING "Use this to also build tests." ON) option(BUILD_INTEGRATION "Use this to also build integration tests." OFF) +option(ASAN "Enable AddressSanitizer." OFF) message(STATUS "Produce Debian and RPM packages: ${PACKAGING}") message(STATUS "Build examples: ${BUILD_EXAMPLES}") message(STATUS "Build tools: ${BUILD_TOOLS}") message(STATUS "Build tests: ${BUILD_TESTING}") message(STATUS "Build integration: ${BUILD_INTEGRATION}") +message(STATUS "AddressSanitizer: ${ASAN}") set(PICO_LIBS "") if(PICO_STATIC) @@ -453,6 +454,11 @@ if(BUILD_EXAMPLES) add_subdirectory(examples) endif() +if(ASAN) + add_compile_options(-fsanitize=address) + add_link_options(-fsanitize=address) +endif() + if(UNIX OR MSVC) if(BUILD_TOOLS) set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/tools) diff --git a/GNUmakefile b/GNUmakefile index 0a254575c..12a97bf06 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -41,6 +41,10 @@ BUILD_TOOLS?=OFF # Accepted values: ON, OFF FORCE_C99?=OFF +# Enable AddressSanitizer. +# Accepted values: ON, OFF +ASAN?=OFF + # Debug level. This sets the ZENOH_DEBUG variable. # Accepted values: # 0: NONE @@ -82,7 +86,7 @@ CMAKE_OPT=-DZENOH_DEBUG=$(ZENOH_DEBUG) -DBUILD_EXAMPLES=$(BUILD_EXAMPLES) -DCMAK -DZ_FEATURE_MULTI_THREAD=$(Z_FEATURE_MULTI_THREAD) -DZ_FEATURE_INTEREST=$(Z_FEATURE_INTEREST) -DZ_FEATURE_UNSTABLE_API=$(Z_FEATURE_UNSTABLE_API)\ -DZ_FEATURE_PUBLICATION=$(Z_FEATURE_PUBLICATION) -DZ_FEATURE_SUBSCRIPTION=$(Z_FEATURE_SUBSCRIPTION) -DZ_FEATURE_QUERY=$(Z_FEATURE_QUERY) -DZ_FEATURE_QUERYABLE=$(Z_FEATURE_QUERYABLE)\ -DZ_FEATURE_RAWETH_TRANSPORT=$(Z_FEATURE_RAWETH_TRANSPORT) -DFRAG_MAX_SIZE=$(FRAG_MAX_SIZE) -DBATCH_UNICAST_SIZE=$(BATCH_UNICAST_SIZE) -DBATCH_MULTICAST_SIZE=$(BATCH_MULTICAST_SIZE)\ - -DBUILD_INTEGRATION=$(BUILD_INTEGRATION) -DBUILD_TOOLS=$(BUILD_TOOLS) -DBUILD_SHARED_LIBS=$(BUILD_SHARED_LIBS) -H. + -DASAN=$(ASAN) -DBUILD_INTEGRATION=$(BUILD_INTEGRATION) -DBUILD_TOOLS=$(BUILD_TOOLS) -DBUILD_SHARED_LIBS=$(BUILD_SHARED_LIBS) -H. ifeq ($(FORCE_C99), ON) CMAKE_OPT += -DCMAKE_C_STANDARD=99 From 3482ecf0c84e94fc81739496d6fc2b3c6a23ad98 Mon Sep 17 00:00:00 2001 From: Alexander Bushnev Date: Fri, 22 Nov 2024 23:18:44 +0100 Subject: [PATCH 2/4] Fix weak reference counting --- src/collections/refcount.c | 3 ++- tests/z_refcount_test.c | 15 ++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/collections/refcount.c b/src/collections/refcount.c index 608548fae..7a44ab4e6 100644 --- a/src/collections/refcount.c +++ b/src/collections/refcount.c @@ -198,7 +198,8 @@ bool _z_rc_decrease_strong(void** cnt) { if (_ZP_RC_OP_DECR_AND_CMP_STRONG(c, 1)) { return _z_rc_decrease_weak(cnt); } - return _z_rc_decrease_weak(cnt); + _z_rc_decrease_weak(cnt); + return true; } bool _z_rc_decrease_weak(void** cnt) { diff --git a/tests/z_refcount_test.c b/tests/z_refcount_test.c index 27cb84d9a..fc8b322cc 100644 --- a/tests/z_refcount_test.c +++ b/tests/z_refcount_test.c @@ -141,7 +141,7 @@ void test_rc_clone_as_weak(void) { assert(_z_rc_weak_count(dwk1._cnt) == 2); assert(dwk1._val->foo == 42); - assert(!_dummy_rc_drop(&drc1)); + assert(_dummy_rc_drop(&drc1)); assert(_z_rc_strong_count(dwk1._cnt) == 0); assert(_z_rc_weak_count(dwk1._cnt) == 1); assert(_dummy_weak_drop(&dwk1)); @@ -156,7 +156,7 @@ void test_rc_clone_as_weak_ptr(void) { assert(_z_rc_strong_count(dwk1->_cnt) == 1); assert(_z_rc_weak_count(dwk1->_cnt) == 2); - assert(!_dummy_rc_drop(&drc1)); + assert(_dummy_rc_drop(&drc1)); assert(_z_rc_strong_count(dwk1->_cnt) == 0); assert(_z_rc_weak_count(dwk1->_cnt) == 1); assert(_dummy_weak_drop(dwk1)); @@ -180,7 +180,7 @@ void test_weak_clone(void) { assert(_z_rc_strong_count(dwk2._cnt) == 1); assert(_z_rc_weak_count(dwk2._cnt) == 3); - assert(!_dummy_rc_drop(&drc1)); + assert(_dummy_rc_drop(&drc1)); assert(_z_rc_strong_count(dwk2._cnt) == 0); assert(_z_rc_weak_count(dwk2._cnt) == 2); @@ -208,7 +208,7 @@ void test_weak_copy(void) { void test_weak_upgrade(void) { _dummy_t val = {.foo = 42}; - _dummy_rc_t drc1 = _dummy_rc_new(&val); + _dummy_rc_t drc1 = _dummy_rc_new_from_val(&val); _dummy_weak_t dwk1 = _dummy_rc_clone_as_weak(&drc1); // Valid upgrade @@ -217,7 +217,7 @@ void test_weak_upgrade(void) { assert(_z_rc_strong_count(drc2._cnt) == 2); assert(_z_rc_weak_count(drc2._cnt) == 3); assert(!_dummy_rc_drop(&drc1)); - assert(!_dummy_rc_drop(&drc2)); + assert(_dummy_rc_drop(&drc2)); // Failed upgrade _dummy_rc_t drc3 = _dummy_weak_upgrade(&dwk1); @@ -240,6 +240,10 @@ void test_overflow(void) { _dummy_weak_t dwk1 = _dummy_rc_clone_as_weak(&drc1); assert(_Z_RC_IS_NULL(&dwk1)); + + // Manual free to make asan happy, without long decresing + free(drc1._val); + free(drc1._cnt); } void test_decr(void) { @@ -248,6 +252,7 @@ void test_decr(void) { _dummy_rc_t drc2 = _dummy_rc_clone(&drc1); assert(!_dummy_rc_decr(&drc2)); assert(_dummy_rc_decr(&drc1)); + free(drc1._val); } int main(void) { From aa0a472c76a9e8661400c931aa11ae9bcf580afb Mon Sep 17 00:00:00 2001 From: Alexander Bushnev Date: Fri, 22 Nov 2024 23:19:40 +0100 Subject: [PATCH 3/4] Fix keyexpr memory access issues --- src/protocol/keyexpr.c | 7 +++---- tests/z_keyexpr_test.c | 2 ++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/protocol/keyexpr.c b/src/protocol/keyexpr.c index e27180f63..ce185f512 100644 --- a/src/protocol/keyexpr.c +++ b/src/protocol/keyexpr.c @@ -146,7 +146,7 @@ zp_keyexpr_canon_status_t __zp_canon_prefix(const char *start, size_t *len) { char const *next_slash; do { - next_slash = strchr(chunk_start, '/'); + next_slash = memchr(chunk_start, '/', _z_ptr_char_diff(end, chunk_start)); const char *chunk_end = next_slash ? next_slash : end; size_t chunk_len = _z_ptr_char_diff(chunk_end, chunk_start); switch (chunk_len) { @@ -291,7 +291,7 @@ void __zp_ke_write_chunk(char **writer, const char *chunk, size_t len, const cha writer[0] = _z_ptr_char_offset(writer[0], 1); } - (void)memcpy(writer[0], chunk, len); + (void)memmove(writer[0], chunk, len); writer[0] = _z_ptr_char_offset(writer[0], (ptrdiff_t)len); } @@ -766,10 +766,9 @@ zp_keyexpr_canon_status_t _z_keyexpr_canonize(char *start, size_t *len) { } else { assert(false); // anything before "$*" or "**" must be part of the canon prefix } - while (next_slash != NULL) { reader = _z_ptr_char_offset(next_slash, 1); - next_slash = strchr(reader, '/'); + next_slash = memchr(reader, '/', _z_ptr_char_diff(end, reader)); chunk_end = next_slash ? next_slash : end; switch (_z_ptr_char_diff(chunk_end, reader)) { case 0: { diff --git a/tests/z_keyexpr_test.c b/tests/z_keyexpr_test.c index 96fc7df93..58b0bdd8f 100644 --- a/tests/z_keyexpr_test.c +++ b/tests/z_keyexpr_test.c @@ -314,6 +314,7 @@ void test_canonize(void) { printf(" Match: %.*s : %s\n", (int)canon_len, canon, canonized[i]); assert(strncmp(canonized[i], canon, canon_len) == 0); } + free(canon); } for (int i = 0; i < N; i++) { @@ -331,6 +332,7 @@ void test_canonize(void) { assert(strncmp(canonized[i], canon, canon_len) == 0); } printf("\n"); + free(canon); } } From b64706afad74943a94b25cd3133663ddb884889a Mon Sep 17 00:00:00 2001 From: Alexander Bushnev Date: Sat, 23 Nov 2024 03:31:37 +0100 Subject: [PATCH 4/4] Fix unitest memory leaks --- tests/z_api_encoding_test.c | 1 + tests/z_channels_test.c | 7 ++++--- tests/z_collections_test.c | 12 +++++++----- tests/z_data_struct_test.c | 6 ++++++ tests/z_endpoint_test.c | 17 +++++++++++++++++ tests/z_msgcodec_test.c | 13 +++++++++---- 6 files changed, 44 insertions(+), 12 deletions(-) diff --git a/tests/z_api_encoding_test.c b/tests/z_api_encoding_test.c index 03a3b0df9..98895a003 100644 --- a/tests/z_api_encoding_test.c +++ b/tests/z_api_encoding_test.c @@ -85,6 +85,7 @@ void test_with_schema(void) { z_encoding_to_string(z_encoding_loan_mut(&e), &s); assert(strncmp("zenoh/bytes;my_schema", z_string_data(z_string_loan(&s)), z_string_len(z_string_loan(&s))) == 0); z_encoding_drop(z_encoding_move(&e)); + z_string_drop(z_string_move(&s)); z_encoding_from_str(&e, "zenoh/string;"); z_encoding_set_schema_from_substr(z_encoding_loan_mut(&e), "my_schema", 3); diff --git a/tests/z_channels_test.c b/tests/z_channels_test.c index 5bd241d53..80ea7e9f4 100644 --- a/tests/z_channels_test.c +++ b/tests/z_channels_test.c @@ -13,12 +13,10 @@ // #include #include -#include -#include #include "zenoh-pico/api/handlers.h" #include "zenoh-pico/api/macros.h" -#include "zenoh-pico/net/sample.h" +#include "zenoh-pico/collections/bytes.h" #undef NDEBUG #include @@ -37,6 +35,7 @@ .attachment = _z_bytes_null(), \ }; \ z_call(*z_loan(closure), &sample); \ + _z_bytes_drop(&payload); \ } while (0); #define _RECV(handler, method, buf) \ @@ -192,11 +191,13 @@ void zero_size_test(void) { assert(z_fifo_channel_sample_new(&closure, &fifo_handler, 0) != Z_OK); assert(z_fifo_channel_sample_new(&closure, &fifo_handler, 1) == Z_OK); z_drop(z_move(fifo_handler)); + z_drop(z_move(closure)); z_owned_ring_handler_sample_t ring_handler; assert(z_ring_channel_sample_new(&closure, &ring_handler, 0) != Z_OK); assert(z_ring_channel_sample_new(&closure, &ring_handler, 1) == Z_OK); z_drop(z_move(ring_handler)); + z_drop(z_move(closure)); } int main(void) { diff --git a/tests/z_collections_test.c b/tests/z_collections_test.c index 8bb54007f..0fe9bab17 100644 --- a/tests/z_collections_test.c +++ b/tests/z_collections_test.c @@ -18,7 +18,6 @@ #include "zenoh-pico/collections/fifo.h" #include "zenoh-pico/collections/lifo.h" -#include "zenoh-pico/collections/list.h" #include "zenoh-pico/collections/ring.h" #include "zenoh-pico/collections/string.h" @@ -313,10 +312,10 @@ void int_map_iterator_test(void) { _z_str_intmap_t map; map = _z_str_intmap_make(); - _z_str_intmap_insert(&map, 10, "A"); - _z_str_intmap_insert(&map, 20, "B"); - _z_str_intmap_insert(&map, 30, "C"); - _z_str_intmap_insert(&map, 40, "D"); + _z_str_intmap_insert(&map, 10, _z_str_clone("A")); + _z_str_intmap_insert(&map, 20, _z_str_clone("B")); + _z_str_intmap_insert(&map, 30, _z_str_clone("C")); + _z_str_intmap_insert(&map, 40, _z_str_clone("D")); #define TEST_MAP(map) \ { \ @@ -346,6 +345,9 @@ void int_map_iterator_test(void) { TEST_MAP(map2); + _z_str_intmap_clear(&map); + _z_str_intmap_clear(&map2); + #undef TEST_MAP } diff --git a/tests/z_data_struct_test.c b/tests/z_data_struct_test.c index e3dff0fa5..045f832e8 100644 --- a/tests/z_data_struct_test.c +++ b/tests/z_data_struct_test.c @@ -15,6 +15,7 @@ #include #include #include +#include #include "zenoh-pico/api/primitives.h" #include "zenoh-pico/api/types.h" @@ -29,6 +30,7 @@ void entry_list_test(void) { _z_transport_peer_entry_list_t *root = _z_transport_peer_entry_list_new(); for (int i = 0; i < 10; i++) { _z_transport_peer_entry_t *entry = (_z_transport_peer_entry_t *)z_malloc(sizeof(_z_transport_peer_entry_t)); + memset(entry, 0, sizeof(_z_transport_peer_entry_t)); root = _z_transport_peer_entry_list_insert(root, entry); } _z_transport_peer_entry_list_t *list = root; @@ -39,6 +41,7 @@ void entry_list_test(void) { for (int i = 0; i < 11; i++) { _z_transport_peer_entry_t *entry = (_z_transport_peer_entry_t *)z_malloc(sizeof(_z_transport_peer_entry_t)); + memset(entry, 0, sizeof(_z_transport_peer_entry_t)); root = _z_transport_peer_entry_list_insert(root, entry); } assert(_z_transport_peer_entry_list_head(root)->_peer_id == _Z_KEYEXPR_MAPPING_UNKNOWN_REMOTE - 1); @@ -140,6 +143,8 @@ void str_vec_list_intmap_test(void) { _z_str_intmap_clear(&map); assert(_z_str_intmap_is_empty(&map) == true); + + z_free(s); } void _z_slice_custom_deleter(void *data, void *context) { @@ -243,6 +248,7 @@ void z_id_to_string_test(void) { assert(z_string_len(z_string_loan(&id_str)) == 32); assert(strncmp("0f0e0d0c0b0a09080706050403020100", z_string_data(z_string_loan(&id_str)), z_string_len(z_string_loan(&id_str))) == 0); + z_string_drop(z_string_move(&id_str)); } int main(void) { diff --git a/tests/z_endpoint_test.c b/tests/z_endpoint_test.c index 4252b0665..c2f97d6ad 100644 --- a/tests/z_endpoint_test.c +++ b/tests/z_endpoint_test.c @@ -42,25 +42,32 @@ int main(void) { str = _z_string_alias_str(""); assert(_z_locator_from_string(&lc, &str) == _Z_ERR_CONFIG_LOCATOR_INVALID); + _z_locator_clear(&lc); str = _z_string_alias_str("/"); assert(_z_locator_from_string(&lc, &str) == _Z_ERR_CONFIG_LOCATOR_INVALID); + _z_locator_clear(&lc); str = _z_string_alias_str("tcp"); assert(_z_locator_from_string(&lc, &str) == _Z_ERR_CONFIG_LOCATOR_INVALID); + _z_locator_clear(&lc); str = _z_string_alias_str("tcp/"); assert(_z_locator_from_string(&lc, &str) == _Z_ERR_CONFIG_LOCATOR_INVALID); + _z_locator_clear(&lc); str = _z_string_alias_str("127.0.0.1:7447"); assert(_z_locator_from_string(&lc, &str) == _Z_ERR_CONFIG_LOCATOR_INVALID); + _z_locator_clear(&lc); str = _z_string_alias_str("tcp/127.0.0.1:7447?"); assert(_z_locator_from_string(&lc, &str) == _Z_RES_OK); + _z_locator_clear(&lc); // No metadata defined so far... but this is a valid syntax in principle str = _z_string_alias_str("tcp/127.0.0.1:7447?invalid=ctrl"); assert(_z_locator_from_string(&lc, &str) == _Z_RES_OK); + _z_locator_clear(&lc); // Endpoint printf(">>> Testing endpoints...\n"); @@ -80,25 +87,32 @@ int main(void) { str = _z_string_alias_str(""); assert(_z_endpoint_from_string(&ep, &str) == _Z_ERR_CONFIG_LOCATOR_INVALID); + _z_endpoint_clear(&ep); str = _z_string_alias_str("/"); assert(_z_endpoint_from_string(&ep, &str) == _Z_ERR_CONFIG_LOCATOR_INVALID); + _z_endpoint_clear(&ep); str = _z_string_alias_str("tcp"); assert(_z_endpoint_from_string(&ep, &str) == _Z_ERR_CONFIG_LOCATOR_INVALID); + _z_endpoint_clear(&ep); str = _z_string_alias_str("tcp/"); assert(_z_endpoint_from_string(&ep, &str) == _Z_ERR_CONFIG_LOCATOR_INVALID); + _z_endpoint_clear(&ep); str = _z_string_alias_str("127.0.0.1:7447"); assert(_z_endpoint_from_string(&ep, &str) == _Z_ERR_CONFIG_LOCATOR_INVALID); + _z_endpoint_clear(&ep); str = _z_string_alias_str("tcp/127.0.0.1:7447?"); assert(_z_endpoint_from_string(&ep, &str) == _Z_RES_OK); + _z_endpoint_clear(&ep); // No metadata defined so far... but this is a valid syntax in principle str = _z_string_alias_str("tcp/127.0.0.1:7447?invalid=ctrl"); assert(_z_endpoint_from_string(&ep, &str) == _Z_RES_OK); + _z_endpoint_clear(&ep); str = _z_string_alias_str("udp/127.0.0.1:7447#iface=eth0"); assert(_z_endpoint_from_string(&ep, &str) == _Z_RES_OK); @@ -116,12 +130,15 @@ int main(void) { str = _z_string_alias_str("udp/127.0.0.1:7447#invalid=eth0"); assert(_z_endpoint_from_string(&ep, &str) == _Z_RES_OK); + _z_endpoint_clear(&ep); str = _z_string_alias_str("udp/127.0.0.1:7447?invalid=ctrl#iface=eth0"); assert(_z_endpoint_from_string(&ep, &str) == _Z_RES_OK); + _z_endpoint_clear(&ep); str = _z_string_alias_str("udp/127.0.0.1:7447?invalid=ctrl#invalid=eth0"); assert(_z_endpoint_from_string(&ep, &str) == _Z_RES_OK); + _z_endpoint_clear(&ep); return 0; } diff --git a/tests/z_msgcodec_test.c b/tests/z_msgcodec_test.c index 59237b3b2..d4bbdd2c4 100644 --- a/tests/z_msgcodec_test.c +++ b/tests/z_msgcodec_test.c @@ -221,18 +221,23 @@ char *gen_str(size_t size) { return str; } +_z_string_t gen_string(size_t len) { + char *str = gen_str(len); + _z_string_t ret = _z_string_copy_from_str(str); + z_free(str); + return ret; +} + _z_string_svec_t gen_str_array(size_t size) { _z_string_svec_t sa = _z_string_svec_make(size); for (size_t i = 0; i < size; i++) { - _z_string_t s = _z_string_copy_from_str(gen_str(16)); + _z_string_t s = gen_string(16); _z_string_svec_append(&sa, &s); } return sa; } -_z_string_t gen_string(size_t len) { return _z_string_alias_str(gen_str(len)); } - _z_locator_array_t gen_locator_array(size_t size) { _z_locator_array_t la = _z_locator_array_make(size); for (size_t i = 0; i < size; i++) { @@ -341,7 +346,7 @@ void assert_eq_locator_array(const _z_locator_array_t *left, const _z_locator_ar _z_string_t ls = _z_locator_to_string(l); _z_string_t rs = _z_locator_to_string(r); - printf("%s:%s", _z_string_data(&ls), _z_string_data(&rs)); + printf("%.*s:%.*s", (int)_z_string_len(&ls), _z_string_data(&ls), (int)_z_string_len(&rs), _z_string_data(&rs)); if (i < left->_len - 1) printf(" "); _z_string_clear(&ls);