From 3376a21cb270f2da1f51a929058451d22a66e93f Mon Sep 17 00:00:00 2001 From: "E. Scott Daniels" Date: Tue, 14 May 2019 14:14:20 +0000 Subject: [PATCH] fix(rtable): Potential memory leak in rte replace If a route table entry was overlaid during the loading of a new table there was a potential for leaking. Added unit tests to better verify results of symtab cloning. Change-Id: I6afecf52e9a48b36da3a06fa01867956a1a52261 Signed-off-by: E. Scott Daniels --- CMakeLists.txt | 2 +- src/common/include/rmr_agnostic.h | 2 + src/common/src/rt_generic_static.c | 94 +++++++++++++++++++++++++++++++++++++- test/rt_static_test.c | 63 +++++++++++++++++++++++-- 4 files changed, 156 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 51639d2..6b73dd9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -23,7 +23,7 @@ cmake_minimum_required( VERSION 3.5 ) set( major_version "1" ) # should be automatically populated from git tag later, but until CI process sets a tag we use this set( minor_version "0" ) -set( patch_level "22" ) +set( patch_level "23" ) set( install_root "${CMAKE_INSTALL_PREFIX}" ) set( install_lib "lib" ) diff --git a/src/common/include/rmr_agnostic.h b/src/common/include/rmr_agnostic.h index 06da060..4b772e3 100644 --- a/src/common/include/rmr_agnostic.h +++ b/src/common/include/rmr_agnostic.h @@ -168,6 +168,8 @@ typedef struct { round robin messags across the list. */ typedef struct { + uint64_t key; // key used to reinsert this entry into a new symtab + int refs; // number of symtabs which reference the entry int mtype; // the message type for this list int nrrgroups; // number of rr groups to send to rrgroup_t** rrgroups; // one or more set of endpoints to round robin messages to diff --git a/src/common/src/rt_generic_static.c b/src/common/src/rt_generic_static.c index 5bbacbd..ef38b8a 100644 --- a/src/common/src/rt_generic_static.c +++ b/src/common/src/rt_generic_static.c @@ -95,6 +95,7 @@ static char* clip( char* buf ) { */ static rtable_ent_t* uta_add_rte( route_table_t* rt, uint64_t key, int nrrgroups ) { rtable_ent_t* rte; + rtable_ent_t* old_rte; // entry which was already in the table for the key if( rt == NULL ) { return NULL; @@ -105,6 +106,7 @@ static rtable_ent_t* uta_add_rte( route_table_t* rt, uint64_t key, int nrrgroups return NULL; } memset( rte, 0, sizeof( *rte ) ); + rte->refs = 1; if( nrrgroups <= 0 ) { nrrgroups = 10; @@ -118,6 +120,10 @@ static rtable_ent_t* uta_add_rte( route_table_t* rt, uint64_t key, int nrrgroups memset( rte->rrgroups, 0, sizeof( rrgroup_t *) * nrrgroups ); rte->nrrgroups = nrrgroups; + if( (old_rte = rmr_sym_pull( rt->hash, key )) != NULL ) { + del_rte( NULL, NULL, NULL, old_rte, NULL ); // dec the ref counter and trash if unreferenced + } + rmr_sym_map( rt->hash, key, rte ); // add to hash using numeric mtype as key if( DEBUG ) fprintf( stderr, "[DBUG] route table entry created: k=%lu groups=%d\n", key, nrrgroups ); @@ -341,6 +347,18 @@ static void collect_things( void* st, void* entry, char const* name, void* thing Called to delete a route table entry struct. We delete the array of endpoint pointers, but NOT the endpoints referenced as those are referenced from multiple entries. + + Route table entries can be concurrently referenced by multiple symtabs, so + the actual delete happens only if decrementing the rte's ref count takes it + to 0. Thus, it is safe to call this function across a symtab when cleaning up + the symtab, or overlaying an entry. + + This function uses ONLY the pointer to the rte (thing) and ignores the other + information that symtab foreach function passes (st, entry, and data) which + means that it _can_ safetly be used outside of the foreach setting. If + the function is changed to depend on any of these three, then a stand-alone + rte_cleanup() function should be added and referenced by this, and refererences + to this outside of the foreach world should be changed. */ static void del_rte( void* st, void* entry, char const* name, void* thing, void* data ) { rtable_ent_t* rte; @@ -350,6 +368,11 @@ static void del_rte( void* st, void* entry, char const* name, void* thing, void* return; } + rte->refs--; + if( rte->refs > 0 ) { // something still referencing, so it lives + return; + } + if( rte->rrgroups ) { // clean up the round robin groups for( i = 0; i < rte->nrrgroups; i++ ) { if( rte->rrgroups[i] ) { @@ -452,7 +475,7 @@ static route_table_t* uta_rt_init( ) { static route_table_t* uta_rt_clone( route_table_t* srt ) { endpoint_t* ep; // an endpoint route_table_t* nrt; // new route table - route_table_t* art; // active route table + //route_table_t* art; // active route table void* sst; // source symtab void* nst; // new symtab thing_list_t things; @@ -494,6 +517,75 @@ static route_table_t* uta_rt_clone( route_table_t* srt ) { return nrt; } +/* + Clones _all_ of the given route table (references both endpoints AND the route table + entries. Needed to support a partial update where some route table entries will not + be deleted if not explicitly in the update. +*/ +static route_table_t* uta_rt_clone_all( route_table_t* srt ) { + endpoint_t* ep; // an endpoint + rtable_ent_t* rte; // a route table entry + route_table_t* nrt; // new route table + //route_table_t* art; // active route table + void* sst; // source symtab + void* nst; // new symtab + thing_list_t things0; // things from space 0 (table entries) + thing_list_t things1; // things from space 1 (end points) + int i; + + if( srt == NULL ) { + return NULL; + } + + if( (nrt = (route_table_t *) malloc( sizeof( *nrt ) )) == NULL ) { + return NULL; + } + + if( (nrt->hash = rmr_sym_alloc( 509 )) == NULL ) { // modest size, prime + free( nrt ); + return NULL; + } + + things0.nalloc = 2048; + things0.nused = 0; + things0.things = (void **) malloc( sizeof( void * ) * things0.nalloc ); + if( things0.things == NULL ) { + free( nrt->hash ); + free( nrt ); + return NULL; + } + + things1.nalloc = 2048; + things1.nused = 0; + things1.things = (void **) malloc( sizeof( void * ) * things1.nalloc ); + if( things1.things == NULL ) { + free( nrt->hash ); + free( nrt ); + return NULL; + } + + sst = srt->hash; // convenience pointers (src symtab) + nst = nrt->hash; + + rmr_sym_foreach_class( sst, 0, collect_things, &things0 ); // collect the rtes + rmr_sym_foreach_class( sst, 1, collect_things, &things1 ); // collect the named endpoints in the active table + + for( i = 0; i < things0.nused; i++ ) { + rte = (rtable_ent_t *) things0.things[i]; + rte->refs++; // rtes can be removed, so we track references + rmr_sym_map( nst, rte->key, rte ); // add to hash using numeric mtype/sub-id as key (default to space 0) + } + + for( i = 0; i < things1.nused; i++ ) { + ep = (endpoint_t *) things1.things[i]; + rmr_sym_put( nst, ep->name, 1, ep ); // slam this one into the new table + } + + free( things0.things ); + free( things1.things ); + return nrt; +} + /* Given a name, find the endpoint struct in the provided route table. */ diff --git a/test/rt_static_test.c b/test/rt_static_test.c index ce0eb25..972b767 100644 --- a/test/rt_static_test.c +++ b/test/rt_static_test.c @@ -44,6 +44,38 @@ typedef struct entry_info { } ei_t; +/* + Driven by symtab foreach element of one space. + We count using the data as a counter. +*/ +static void count_things( void* st, void* entry, char const* name, void* thing, void* vdata ) { + int* counter; + + if( thing ) { + if( (counter = (int *) vdata) != NULL ) { + *counter++; + } + } +} + +/* + Returns the number of entries in the table for the given class. +*/ +static int count_entries( route_table_t* rt, int class ) { + int counter = 0; + + if( ! rt ) { + return 0; + } + if( !rt->hash ) { + return 0; + } + + rmr_sym_foreach_class( rt->hash, class, count_things, &counter ); // run each and update counter + + return counter; +} + /* This is the main route table test. It sets up a very specific table for testing (not via the generic setup function for other test @@ -59,6 +91,8 @@ static int rt_test( ) { int errors = 0; // number errors found int i; int k; + int c1; // general counters + int c2; int mtype; int value; int alt_value; @@ -142,8 +176,33 @@ static int rt_test( ) { } } - crt = uta_rt_clone( rt ); + crt = uta_rt_clone( rt ); // clone only the endpoint entries errors += fail_if_nil( crt, "cloned route table" ); + if( crt ) { + c1 = count_entries( rt, 1 ); + c2 = count_entries( crt, 1 ); + errors += fail_not_equal( c1, c2, "cloned (endpoints) table entries space 1 count (b) did not match original table count (a)" ); + + c2 = count_entries( crt, 0 ); + errors += fail_not_equal( c2, 0, "cloned (endpoints) table entries space 0 count (a) was not zero as expected" ); + uta_rt_drop( crt ); + } + + + crt = uta_rt_clone_all( rt ); // clone all entries + errors += fail_if_nil( crt, "cloned all route table" ); + + if( crt ) { + c1 = count_entries( rt, 0 ); + c2 = count_entries( crt, 0 ); + errors += fail_not_equal( c1, c2, "cloned (all) table entries space 0 count (b) did not match original table count (a)" ); + + c1 = count_entries( rt, 1 ); + c2 = count_entries( crt, 1 ); + errors += fail_not_equal( c1, c2, "cloned (all) table entries space 1 count (b) did not match original table count (a)" ); + uta_rt_drop( crt ); + } + ep = uta_get_ep( rt, "localhost:4561" ); errors += fail_if_nil( ep, "end point (fetch by name)" ); @@ -198,8 +257,6 @@ static int rt_test( ) { uta_rt_drop( rt ); - uta_rt_drop( crt ); - if( (ctx = (uta_ctx_t *) malloc( sizeof( uta_ctx_t ) )) != NULL ) { memset( ctx, 0, sizeof( *ctx ) ); -- 2.16.6