From 0a58458afe875798f0ac3f367bd05e4ba523e115 Mon Sep 17 00:00:00 2001 From: "E. Scott Daniels" Date: Mon, 6 Apr 2020 16:42:32 -0400 Subject: [PATCH] Fix SI address and initialistion bugs The SI95 address translation between an address struct and human readable string had a function call with reversed parameter constants. The SI initialisation could excite an NPE if the v6 "listen all" string (::) was given instead of 0.0.0.0. Issue-ID: RIC-327 Signed-off-by: E. Scott Daniels Change-Id: Ifa673aca7ceaa7e47fbd9158340f0f825ac0fae8 --- CHANGES_CORE.txt | 4 +++ CMakeLists.txt | 2 +- src/rmr/si/src/rmr_si.c | 2 +- src/rmr/si/src/si95/siaddress.c | 18 +++++----- test/si95_test.c | 74 ++++++++++++++++++++++++++++++++++++++--- test/wormhole_static_test.c | 41 +++++++++++++++++++++++ 6 files changed, 127 insertions(+), 14 deletions(-) diff --git a/CHANGES_CORE.txt b/CHANGES_CORE.txt index a8b20b7..c3244be 100644 --- a/CHANGES_CORE.txt +++ b/CHANGES_CORE.txt @@ -5,6 +5,10 @@ # API and build change and fix summaries. Doc correctsions # and/or changes are not mentioned here; see the commit messages. +2020 April 6; version 3.6.6 + Correct bug in SI95 address conversion module (RIC-327) + Correct bug in SI initialisation module + 2020 April 2; version 3.6.5 Correct potential nil pointer use when examining interfaces for use as a listen target (RIC-307) diff --git a/CMakeLists.txt b/CMakeLists.txt index e2392c1..b54c676 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -40,7 +40,7 @@ cmake_minimum_required( VERSION 3.5 ) set( major_version "3" ) # should be automatically populated from git tag later, but until CI process sets a tag we use this set( minor_version "6" ) -set( patch_level "5" ) +set( patch_level "6" ) set( install_root "${CMAKE_INSTALL_PREFIX}" ) set( install_inc "include/rmr" ) diff --git a/src/rmr/si/src/rmr_si.c b/src/rmr/si/src/rmr_si.c index 8606241..17014a6 100644 --- a/src/rmr/si/src/rmr_si.c +++ b/src/rmr/si/src/rmr_si.c @@ -674,7 +674,7 @@ static void* init( char* uproto_port, int max_msg_size, int flags ) { ctx->my_ip = get_default_ip( ctx->ip_list ); // and (guess) at what should be the default to put into messages as src if( ctx->my_ip == NULL ) { rmr_vlog( RMR_VL_WARN, "rmr_init: default ip address could not be sussed out, using name\n" ); - strcpy( ctx->my_ip, ctx->my_name ); // if we cannot suss it out, use the name rather than a nil pointer + ctx->my_ip = strdup( ctx->my_name ); // if we cannot suss it out, use the name rather than a nil pointer } } if( DEBUG ) rmr_vlog( RMR_VL_DEBUG, " default ip address: %s\n", ctx->my_ip ); diff --git a/src/rmr/si/src/si95/siaddress.c b/src/rmr/si/src/si95/siaddress.c index 8761b66..e77ab4f 100644 --- a/src/rmr/si/src/si95/siaddress.c +++ b/src/rmr/si/src/si95/siaddress.c @@ -86,7 +86,7 @@ extern int SIgenaddr( char *target, int proto, int family, int socktype, struct if( *dstr == '[' ) { // strip [ and ] from v6 and point pstring if port there dstr++; pstr = strchr( dstr, ']' ); - if( *pstr != ']' ) { + if( !pstr || *pstr != ']' ) { free( fptr ); return -1; } @@ -109,15 +109,16 @@ extern int SIgenaddr( char *target, int proto, int family, int socktype, struct memset( &hint, 0, sizeof( hint ) ); hint.ai_family = family; // AF_INET AF_INET6... let this be 0 to select best based on addr hint.ai_socktype = socktype; // SOCK_DGRAM SOCK_STREAM - hint.ai_protocol = proto; // IPPORTO_TCP IPPROTO_UDP + hint.ai_protocol = proto; // IPPROTO_TCP IPPROTO_UDP hint.ai_flags = ga_flags; if( DEBUG ) - rmr_vlog( RMR_VL_DEBUG, "siaddress: calling getaddrinfo flags=%x proto=%d family=%d target=%s host=%s port=%s\n", - ga_flags, proto, family, target, dstr, pstr ); + rmr_vlog( RMR_VL_DEBUG, "siaddress: calling getaddrinfo flags=%x sockty=%d proto=%d family=%d target=%s host=%s port=%s\n", + ga_flags, socktype, proto, family, target, dstr, pstr ); if( (error = getaddrinfo( dstr, pstr, &hint, &list )) ) { - fprintf( stderr, "error from getaddrinfo: target=%s host=%s port=%s(port): error=(%d) %s\n", target, dstr, pstr, error, gai_strerror( error ) ); + fprintf( stderr, "sigenaddr: error from getaddrinfo: target=%s host=%s port=%s(port): error=(%d) %s\n", + target, dstr, pstr, error, gai_strerror( error ) ); } else { *rap = (struct sockaddr *) malloc( list->ai_addrlen ); // alloc a buffer and give address to caller memcpy( *rap, list->ai_addr, list->ai_addrlen ); @@ -167,11 +168,12 @@ extern int SIaddress( void *src, void **dest, int type ) { rlen = strlen( *dest ); break; - case AC_TOADDR6: // from hostname;port string to address for send etc - return SIgenaddr( src, PF_INET6, IPPROTO_TCP, SOCK_STREAM, (struct sockaddr **) dest ); + case AC_TOADDR6: // from hostname:port string to address for send etc + return SIgenaddr( src, IPPROTO_TCP, AF_INET6, SOCK_STREAM, (struct sockaddr **) dest ); case AC_TOADDR: // from dotted decimal to address struct ip4 - return SIgenaddr( src, PF_INET, IPPROTO_TCP, SOCK_STREAM, (struct sockaddr **) dest ); + //return SIgenaddr( src, AF_INET, IPPROTO_TCP, SOCK_STREAM, (struct sockaddr **) dest ); + return SIgenaddr( src, IPPROTO_TCP, AF_INET, SOCK_STREAM, (struct sockaddr **) dest ); } return rlen; diff --git a/test/si95_test.c b/test/si95_test.c index d5bcc3a..26b1b21 100644 --- a/test/si95_test.c +++ b/test/si95_test.c @@ -36,6 +36,12 @@ #include #include +#include // these four needed for si address tests +#include +#include +#include + + #include #include @@ -67,14 +73,14 @@ #include #include -//#include +#include //#include //#include //#include //#include //#include -//#include -//#include +#include +#include //#include //#include #include @@ -90,6 +96,11 @@ #include //#include +// --------------------------------------------------------------------- + +void* si_ctx = NULL; // a global context might be useful + +// --------------------------------------------------------------------- /* Memory allocation/free related tests @@ -124,7 +135,6 @@ static int memory( ) { return errors; } -void* si_ctx = NULL; // a global context might be useful /* Test initialisation related things @@ -141,6 +151,58 @@ static int init() { return errors; } +static int cleanup() { + int errors = 0; + + if( ! si_ctx ) { + return 0; + } + + SIconnect( si_ctx, "localhost:43086" ); // ensure context has a tp block to free on shutdown + SIshutdown( si_ctx ); + + return errors; +} + +/* + Address related tests. +*/ +static int addr() { + int errors = 0; + int l; + struct sockaddr* addr; + char buf1[4096]; + char buf2[4096]; + char* dest; + + addr = (struct sockaddr *) malloc( sizeof( struct sockaddr ) ); +/* + l = SIgenaddr( " [ff02::4]:4567", PF_INET6, IPPROTO_TCP, SOCK_STREAM, &addr ); + + SIgenaddr( " [ff02::4]:4567", PF_INET6, IPPROTO_TCP, SOCK_STREAM, &addr ); +*/ + + dest = NULL; + snprintf( buf1, sizeof( buf1 ), " [ff02::5:4001" ); // invalid address, drive leading space eater too + l = SIaddress( buf1, &dest, AC_TOADDR6 ); + errors += fail_if_true( l > 0, "to addr6 with bad addr convdersion returned valid len" ); + + snprintf( buf1, sizeof( buf1 ), "[ff02::5]:4002" ); // v6 might not be supported so failure is OK here + l=SIaddress( buf1, &dest, AC_TOADDR6 ); + errors += fail_if_true( l < 1, "to addr convdersion failed" ); + + snprintf( buf1, sizeof( buf1 ), "localhost:43086" ); + l = SIaddress( buf1, (void **) &dest, AC_TOADDR ); + errors += fail_if_true( l < 1, "to addr convdersion failed" ); + + snprintf( buf1, sizeof( buf1 ), "localhost:4004" ); + l = SIaddress( buf1, &dest, AC_TODOT ); + errors += fail_if_true( l < 1, "to dot convdersion failed" ); + + return errors; + +} + /* Drive tests... */ @@ -153,7 +215,11 @@ int main() { errors += init(); errors += memory(); + errors += addr(); +fprintf( stderr, ">>> cleaning\n" ); + errors += cleanup(); + fprintf( stderr, " testing finished\n" ); if( errors == 0 ) { fprintf( stderr, " all tests were OK\n\n" ); } else { diff --git a/test/wormhole_static_test.c b/test/wormhole_static_test.c index 25d2cca..9d5fd58 100644 --- a/test/wormhole_static_test.c +++ b/test/wormhole_static_test.c @@ -54,6 +54,7 @@ static int worm_test( ) { void* p; rmr_mbuf_t* mbuf; // mbuf to send to peer + rmr_mbuf_t* resp; // response message from a wormhole call int whid = -1; int last_whid; @@ -63,6 +64,20 @@ static int worm_test( ) { gen_rt( ctx ); + // ---- must run these checks before wormhole(s) are opened -------------------------- + mbuf = rmr_alloc_msg( ctx, 2048 ); // get an muf to pass round, no need to init for first test + resp = rmr_wh_call( ctx, 0, mbuf, 1, 10 ); + errors += fail_if_nil( resp, "wormhole call given valid context and msg, before wormholes open didn't return message pointer" ); + if( resp ) { + errors += fail_if_equal( resp->state, RMR_OK, "wormhole call given valid context and msg, before wormholes open returned ok state" ); + } + + rmr_wh_close( NULL, 0 ); // drive for coverage; nothing to vet + rmr_wh_close( ctx, 0 ); + wh_init( NULL ); + + // ----- end must run before opening wormholes --------------------------------------- + whid = rmr_wh_open( NULL, NULL ); errors += fail_not_equal( whid, -1, "call to wh_open with invalid values did not return bad whid" ); @@ -76,6 +91,9 @@ static int worm_test( ) { whid = rmr_wh_open( ctx, "localhost:89219" ); errors += fail_if_equal( whid, -1, "call to wh_open with valid target failed" ); + i = wh_init( ctx ); // already initialised, ensure no harm + errors += fail_not_equal( i, 1, "second call to wh_init() didn't return true" ); + rmr_wh_close( ctx, 4 ); // test for coverage only; [5] should have nil pointer rmr_wh_close( ctx, 50 ); // test for coverage only; more than allocated reference @@ -153,6 +171,28 @@ static int worm_test( ) { whid = -1; } + // ----- wormhole call -------------------------------------------------- + mbuf = rmr_alloc_msg( ctx, 2048 ); // ensure we have a buffer to pass + resp = rmr_wh_call( NULL, 0, NULL, 1, 10 ); // ensure no msg when msg is nil + errors += fail_not_nil( resp, "wormhole call given nil context and msg didn't return nil message" ); + + resp = rmr_wh_call( NULL, 0, mbuf, 1, 10 ); // ensure invalid state when context is bad + errors += fail_if_nil( resp, "wormhole call given nil context and valid msg didn't return message pointer" ); + if( resp != NULL ) { + errors += fail_if_equal( resp->state, RMR_OK, "wormhole call given nil context and valid message returned OK state" ); + } + + resp = rmr_wh_call( ctx, 2000, mbuf, 1, 10 ); // invalid wormhole id + errors += fail_if_nil( resp, "wormhole call given bad whid returned no msg" ); + if( resp ) { + errors += fail_if_equal( resp->state, RMR_OK, "wormhole call given bad whid returned good state" ); + } + + whid = rmr_wh_open( ctx, "localhost:9219" ); // ensure one is open + resp = rmr_wh_call( ctx, whid, mbuf, 1, 1 ); + errors += fail_if_nil( resp, "wormhole call given valid info returned nil response" ); + + // ----------------------------------------------------------------------- // WARNING: these tests destroy the context, so they MUST be last if( mbuf ) { // only if we got an mbuf @@ -163,6 +203,7 @@ static int worm_test( ) { errors += fail_not_equal( mbuf->state, RMR_ERR_NOHDR, "send with bad header did now set msg state correctly" ); errno = 0; + wh_nuke( NULL ); // coverage only wh_nuke( ctx ); ctx->wormholes = NULL; mbuf = rmr_wh_send_msg( ctx, 4, mbuf ); // coverage test on mbuf header check -- 2.16.6