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 <daniels@research.att.com>
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 51639d2..6b73dd9 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -23,7 +23,7 @@
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 @@
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 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 @@
return NULL;
}
memset( rte, 0, sizeof( *rte ) );
+ rte->refs = 1;
if( nrrgroups <= 0 ) {
nrrgroups = 10;
@@ -118,6 +120,10 @@
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 @@
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 @@
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_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;
@@ -495,6 +518,75 @@
}
/*
+ 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.
*/
static endpoint_t* uta_get_ep( route_table_t* rt, char const* ep_name ) {
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
@@ -45,6 +45,38 @@
/*
+ 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
situations).
@@ -59,6 +91,8 @@
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 @@
}
}
- 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 @@
uta_rt_drop( rt );
- uta_rt_drop( crt );
-
if( (ctx = (uta_ctx_t *) malloc( sizeof( uta_ctx_t ) )) != NULL ) {
memset( ctx, 0, sizeof( *ctx ) );