From 3d7285ec4c96724b64968f46c2075b77e8d08543 Mon Sep 17 00:00:00 2001 From: "E. Scott Daniels" Date: Thu, 5 Mar 2020 16:16:09 -0500 Subject: [PATCH] Address code analysis issues This change addresses the bugs which Sonar identified on 5 March 2020. These were identified as Reliability bugs two of which were found to be true issues; all were addressed in hopes of keeping sonar quiet. Issue-ID: RICAPP-78 Signed-off-by: E. Scott Daniels Change-Id: Id575817686395bbad9683b1df476e3e1e7fbd2b2 --- src/rmr/common/src/rt_generic_static.c | 5 ++- src/rmr/common/src/symtab.c | 65 +++++++++++++++++++++++++++------- src/rmr/common/src/wrapper.c | 9 +++-- src/rmr/nng/src/rmr_nng.c | 11 +++--- src/rmr/nng/src/sr_nng_static.c | 4 +-- src/rmr/si/src/rmr_si.c | 8 +++-- src/rmr/si/src/si95/siaddress.c | 19 +++++----- src/rmr/si/src/si95/sibldpoll.c | 8 +++-- src/rmr/si/src/si95/sipoll.c | 7 ++-- src/rmr/si/src/si95/sitrash.c | 7 ++-- src/rmr/si/src/si95/siwait.c | 5 ++- src/rmr/si/src/sr_si_static.c | 5 +-- 12 files changed, 111 insertions(+), 42 deletions(-) diff --git a/src/rmr/common/src/rt_generic_static.c b/src/rmr/common/src/rt_generic_static.c index ca45f4b..3fc7ded 100644 --- a/src/rmr/common/src/rt_generic_static.c +++ b/src/rmr/common/src/rt_generic_static.c @@ -897,7 +897,8 @@ static void read_static_rt( uta_ctx_t* ctx, int vlevel ) { } } - for( rec = fbuf; rec && *rec; rec = eor+1 ) { + rec = fbuf; + while( rec && *rec ) { rcount++; if( (eor = strchr( rec, '\n' )) != NULL ) { *eor = 0; @@ -909,6 +910,8 @@ static void read_static_rt( uta_ctx_t* ctx, int vlevel ) { } parse_rt_rec( ctx, NULL, rec, vlevel ); // no pvt context as we can't ack + + rec = eor+1; } if( DEBUG ) rmr_vlog_force( RMR_VL_DEBUG, "rmr_read_static: seed route table successfully parsed: %d records\n", rcount ); diff --git a/src/rmr/common/src/symtab.c b/src/rmr/common/src/symtab.c index a105187..18cf60a 100644 --- a/src/rmr/common/src/symtab.c +++ b/src/rmr/common/src/symtab.c @@ -96,7 +96,10 @@ static int sym_hash( const char *n, long size ) return (int ) (t % size); } -/* delete element pointed to by eptr at hash loc hv */ +/* + Delete element pointed to by eptr which is assumed to be + a member of the list at symtab[i]. +*/ static void del_ele( Sym_tab *table, int hv, Sym_ele *eptr ) { Sym_ele **sym_tab; @@ -124,6 +127,39 @@ static void del_ele( Sym_tab *table, int hv, Sym_ele *eptr ) } } +/* + Delete the head element from table[i]. This isn't really + needed, but keeps code analysers from claiming that memory + is being used after it is freed. +*/ +static void del_head_ele( Sym_tab *table, int hv ) { + Sym_ele **sym_tab; + Sym_ele *eptr; // first in list + + + if( hv < 0 || hv >= table->size ) { + return; + } + + sym_tab = table->symlist; + if( (eptr = sym_tab[hv]) != NULL ) // not an empty element; yank it off + { + if( (sym_tab[hv] = eptr->next) != NULL ) { // bump list to next if not the only thing here + sym_tab[hv]->prev = NULL; // new head + } + eptr->next = NULL; // take no chances + + if( eptr->class && eptr->name ) { // class 0 entries are numeric, so name is NOT a pointer + free( (void *) eptr->name ); + } + + free( eptr ); + + table->deaths++; + table->inhabitants--; + } +} + /* Determine if these are the same. */ @@ -205,9 +241,11 @@ extern void rmr_sym_clear( void *vtable ) table = (Sym_tab *) vtable; sym_tab = table->symlist; - for( i = 0; i < table->size; i++ ) - while( sym_tab[i] ) - del_ele( table, i, sym_tab[i] ); + for( i = 0; i < table->size; i++ ) { + while( sym_tab[i] ) { + del_head_ele( table, i ); // delete the head element (and keep bloody sonar from claiming use after free) + } + } } /* @@ -468,15 +506,16 @@ extern void rmr_sym_foreach_class( void *vst, unsigned int class, void (* user_f st = (Sym_tab *) vst; - if( st && (list = st->symlist) != NULL && user_fun != NULL ) - for( i = 0; i < st->size; i++ ) - for( se = list[i]; se; se = next ) /* using next allows user to delete via this */ - { - if( se ) { - next = se->next; - if( class == se->class ) { - user_fun( st, se, se->name, se->val, user_data ); - } + if( st && (list = st->symlist) != NULL && user_fun != NULL ) { + for( i = 0; i < st->size; i++ ) { + se = list[i]; + while( se ) { + next = se->next; // allow callback to delete from the list w/o borking us + if( class == se->class ) { + user_fun( st, se, se->name, se->val, user_data ); } + se = next; } + } + } } diff --git a/src/rmr/common/src/wrapper.c b/src/rmr/common/src/wrapper.c index 18d2a23..55cfe9e 100644 --- a/src/rmr/common/src/wrapper.c +++ b/src/rmr/common/src/wrapper.c @@ -63,8 +63,9 @@ static char* build_sval( char* name, char* val, int add_sep ) { strcat src is freed as a convenience. Max is the max amount that target can accept; we don't bang on if src len is larger than max. Return is the size of src; 0 if the - target was not modified. If target is not modified, then - src is NOT released. + target was not modified. + + Source is ALWAYS freed! */ static int bang_on( char* target, char* src, int max ) { int len; @@ -74,10 +75,12 @@ static int bang_on( char* target, char* src, int max ) { len = strlen( src ); if( (rc = len <= max ? len : 0 ) > 0 ) { // if it fits, add it. strcat( target, src ); - free( src ); } } + if( src ) { + free( src ); + } return rc; } diff --git a/src/rmr/nng/src/rmr_nng.c b/src/rmr/nng/src/rmr_nng.c index faae333..3b590fa 100644 --- a/src/rmr/nng/src/rmr_nng.c +++ b/src/rmr/nng/src/rmr_nng.c @@ -700,6 +700,7 @@ static void* init( char* uproto_port, int max_msg_size, int flags ) { } else { if( (gethostname( wbuf, sizeof( wbuf ) )) != 0 ) { rmr_vlog( RMR_VL_CRIT, "rmr_init: cannot determine localhost name: %s\n", strerror( errno ) ); + free( proto_port ); return NULL; } if( (tok = strchr( wbuf, '.' )) != NULL ) { @@ -726,7 +727,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 ); @@ -747,6 +748,7 @@ static void* init( char* uproto_port, int max_msg_size, int flags ) { if( (state = nng_listen( ctx->nn_sock, bind_info, NULL, NO_FLAGS )) != 0 ) { rmr_vlog( RMR_VL_CRIT, "rmr_init: unable to start nng listener for %s: %s\n", bind_info, nng_strerror( state ) ); nng_close( ctx->nn_sock ); + free( proto_port ); free_ctx( ctx ); return NULL; } @@ -1068,7 +1070,7 @@ extern rmr_mbuf_t* rmr_mt_call( void* vctx, rmr_mbuf_t* mbuf, int call_id, int m } } - state = 0; + state = -1; errno = 0; while( chute->mbuf == NULL && ! errno ) { if( seconds ) { @@ -1094,8 +1096,9 @@ extern rmr_mbuf_t* rmr_mt_call( void* vctx, rmr_mbuf_t* mbuf, int call_id, int m return NULL; // leave errno as set by sem wait call } - mbuf = chute->mbuf; - mbuf->state = RMR_OK; + if( (mbuf = chute->mbuf) != NULL ) { + mbuf->state = RMR_OK; + } chute->mbuf = NULL; return mbuf; diff --git a/src/rmr/nng/src/sr_nng_static.c b/src/rmr/nng/src/sr_nng_static.c index dd978a4..bb45ac6 100644 --- a/src/rmr/nng/src/sr_nng_static.c +++ b/src/rmr/nng/src/sr_nng_static.c @@ -308,7 +308,7 @@ static inline rmr_mbuf_t* clone_msg( rmr_mbuf_t* old_msg ) { nm->alloc_len = mlen; // length of allocated payload if( DEBUG ) rmr_vlog( RMR_VL_DEBUG, "clone values: mty=%d sid=%d len=%d alloc=%d\n", nm->mtype, nm->sub_id, nm->len, nm->alloc_len ); - nm->xaction = hdr->xid; // reference xaction + nm->xaction = &hdr->xid[0]; // point at transaction id in header area nm->state = old_msg->state; // fill in caller's state (likely the state of the last operation) nm->flags = old_msg->flags | MFL_ZEROCOPY; // this is a zerocopy sendable message memcpy( nm->payload, old_msg->payload, old_msg->len ); @@ -377,7 +377,7 @@ static inline rmr_mbuf_t* realloc_msg( rmr_mbuf_t* old_msg, int tr_len ) { nm->len = old_msg->len; // length of data in the payload nm->alloc_len = mlen; // length of allocated payload - nm->xaction = hdr->xid; // reference xaction + nm->xaction = &hdr->xid[0]; // point at transaction id in header area nm->state = old_msg->state; // fill in caller's state (likely the state of the last operation) nm->flags = old_msg->flags | MFL_ZEROCOPY; // this is a zerocopy sendable message memcpy( nm->payload, old_msg->payload, old_msg->len ); diff --git a/src/rmr/si/src/rmr_si.c b/src/rmr/si/src/rmr_si.c index fa46ad7..047401d 100644 --- a/src/rmr/si/src/rmr_si.c +++ b/src/rmr/si/src/rmr_si.c @@ -875,7 +875,9 @@ extern rmr_mbuf_t* rmr_mt_rcv( void* vctx, rmr_mbuf_t* mbuf, int max_wait ) { } } - mbuf->flags |= MFL_ADDSRC; // turn on so if user app tries to send this buffer we reset src + if( mbuf != NULL ) { + mbuf->flags |= MFL_ADDSRC; // turn on so if user app tries to send this buffer we reset src + } return mbuf; } @@ -1056,7 +1058,9 @@ extern rmr_mbuf_t* rmr_mt_call( void* vctx, rmr_mbuf_t* mbuf, int call_id, int m } mbuf = chute->mbuf; - mbuf->state = RMR_OK; + if( mbuf != NULL ) { + mbuf->state = RMR_OK; + } chute->mbuf = NULL; return mbuf; diff --git a/src/rmr/si/src/si95/siaddress.c b/src/rmr/si/src/si95/siaddress.c index 58a3a3d..8761b66 100644 --- a/src/rmr/si/src/si95/siaddress.c +++ b/src/rmr/si/src/si95/siaddress.c @@ -87,6 +87,7 @@ extern int SIgenaddr( char *target, int proto, int family, int socktype, struct dstr++; pstr = strchr( dstr, ']' ); if( *pstr != ']' ) { + free( fptr ); return -1; } @@ -126,7 +127,7 @@ extern int SIgenaddr( char *target, int proto, int family, int socktype, struct freeaddrinfo( list ); // ditch system allocated memory } - free( dstr ); + free( fptr ); return rlen; } @@ -139,7 +140,9 @@ extern int SIgenaddr( char *target, int proto, int family, int socktype, struct */ extern int SIaddress( void *src, void **dest, int type ) { struct sockaddr_in *addr; // pointer to the address + struct sockaddr_in6 *addr6; // ip6 has a different layout unsigned char *num; // pointer at the address number + uint8_t* byte; // pointer at the ipv6 address byte values char wbuf[256]; // work buffer int i; int rlen = 0; // return len - len of address struct or string @@ -147,14 +150,16 @@ extern int SIaddress( void *src, void **dest, int type ) { switch( type ) { case AC_TODOT: // convert from a struct to human readable "dotted decimal" addr = (struct sockaddr_in *) src; - num = (char *) &addr->sin_addr.s_addr; // point at the long if( addr->sin_family == AF_INET6 ) { - sprintf( wbuf, "[%u:%u:%u:%u:%u:%u]:%d", - *(num+0), *(num+1), *(num+2), - *(num+3), *(num+4), *(num+5) , - (int) ntohs( addr->sin_port ) ); + addr6 = (struct sockaddr_in6 *) src; // really an ip6 struct + byte = (uint8_t *) &addr6->sin6_addr; + sprintf( wbuf, "[%u:%u:%u:%u:%u:%u]:%d", + *(byte+0), *(byte+1), *(byte+2), + *(byte+3), *(byte+4), *(byte+5) , + (int) ntohs( addr6->sin6_port ) ); } else { + num = (char *) &addr->sin_addr.s_addr; // point at the long sprintf( wbuf, "%u.%u.%u.%u;%d", *(num+0), *(num+1), *(num+2), *(num+3), (int) ntohs(addr->sin_port) ); } @@ -164,11 +169,9 @@ extern int SIaddress( void *src, void **dest, int type ) { case AC_TOADDR6: // from hostname;port string to address for send etc return SIgenaddr( src, PF_INET6, IPPROTO_TCP, SOCK_STREAM, (struct sockaddr **) dest ); - break; case AC_TOADDR: // from dotted decimal to address struct ip4 return SIgenaddr( src, PF_INET, IPPROTO_TCP, SOCK_STREAM, (struct sockaddr **) dest ); - break; } return rlen; diff --git a/src/rmr/si/src/si95/sibldpoll.c b/src/rmr/si/src/si95/sibldpoll.c index 5b50fcc..8181738 100644 --- a/src/rmr/si/src/si95/sibldpoll.c +++ b/src/rmr/si/src/si95/sibldpoll.c @@ -49,8 +49,10 @@ extern void SIbldpoll( struct ginfo_blk* gptr ) { FD_ZERO( &gptr->writefds ); FD_ZERO( &gptr->execpfds ); - for( tpptr = gptr->tplist; tpptr != NULL; tpptr = nextb ) { - nextb = tpptr->next; + tpptr = gptr->tplist; + while( tpptr != NULL ) { + nextb = tpptr->next; // point past allowing for a delete + if( tpptr->flags & TPF_DELETE ) { if( tpptr->fd >= 0 ) { // wasn't closed for some reason SIterm( gptr, tpptr ); @@ -73,5 +75,7 @@ extern void SIbldpoll( struct ginfo_blk* gptr ) { } } } + + tpptr = nextb; } } diff --git a/src/rmr/si/src/si95/sipoll.c b/src/rmr/si/src/si95/sipoll.c index 0ecca66..c47fb97 100644 --- a/src/rmr/si/src/si95/sipoll.c +++ b/src/rmr/si/src/si95/sipoll.c @@ -121,8 +121,9 @@ extern int SIpoll( struct ginfo_blk *gptr, int msdelay ) // for( tpptr = gptr->tplist; tpptr != NULL; tpptr = tpptr->next ) for( tpptr = gptr->tplist; tpptr != NULL; tpptr = nextone ) - { - nextone = tpptr->next; // prevent coredump if we delete the session + tpptr = gptr->tplist; + while( tpptr != NULL ) { + nextone = tpptr->next; // allow for a delete in loop if( tpptr->squeue != NULL && (FD_ISSET( tpptr->fd, &gptr->writefds )) ) SIsend( gptr, tpptr ); // send if clear to send @@ -180,6 +181,8 @@ extern int SIpoll( struct ginfo_blk *gptr, int msdelay ) } } // end tcp read } // end if event on this fd + + tpptr = nextone; } // end for each fd in the list } // end if not in shutdown diff --git a/src/rmr/si/src/si95/sitrash.c b/src/rmr/si/src/si95/sitrash.c index efc2cd3..679e2f4 100644 --- a/src/rmr/si/src/si95/sitrash.c +++ b/src/rmr/si/src/si95/sitrash.c @@ -51,12 +51,15 @@ extern void SItrash( int type, void *bp ) case TP_BLK: // we assume its off the list tp = (struct tp_blk *) bp; - for( iptr = tp->squeue; iptr; iptr = inext ) // trash any pending send buffers - { + iptr = tp->squeue; + while( iptr != NULL ) { inext = iptr->next; + free( iptr->data ); // we could recurse, but that seems silly free( iptr->addr ); free( iptr ); + + iptr = inext; } if( tp->fd >= 0 ) { diff --git a/src/rmr/si/src/si95/siwait.c b/src/rmr/si/src/si95/siwait.c index 0d1e3e5..6a72b30 100644 --- a/src/rmr/si/src/si95/siwait.c +++ b/src/rmr/si/src/si95/siwait.c @@ -93,7 +93,8 @@ extern int SIwait( struct ginfo_blk *gptr ) { } if( pstat > 0 && (! (gptr->flags & GIF_SHUTDOWN)) ) { - for( tpptr = gptr->tplist; tpptr != NULL; tpptr = nextone ) { + tpptr = gptr->tplist; + while( tpptr != NULL ) { nextone = tpptr->next; // prevent issues if we delete the block during loop if( tpptr->fd >= 0 ) { @@ -129,6 +130,8 @@ extern int SIwait( struct ginfo_blk *gptr ) { } } } // if still good fd + + tpptr = nextone; } } } while( gptr->tplist != NULL && !(gptr->flags & GIF_SHUTDOWN) ); diff --git a/src/rmr/si/src/sr_si_static.c b/src/rmr/si/src/sr_si_static.c index 8f2c6f1..58e1e17 100644 --- a/src/rmr/si/src/sr_si_static.c +++ b/src/rmr/si/src/sr_si_static.c @@ -328,7 +328,7 @@ static inline rmr_mbuf_t* clone_msg( rmr_mbuf_t* old_msg ) { nm->len = old_msg->len; // length of data in the payload nm->alloc_len = mlen; // length of allocated payload - nm->xaction = hdr->xid; // reference xaction + nm->xaction = &hdr->xid[0]; // reference xaction nm->state = old_msg->state; // fill in caller's state (likely the state of the last operation) nm->flags = old_msg->flags | MFL_ZEROCOPY; // this is a zerocopy sendable message memcpy( nm->payload, old_msg->payload, old_msg->len ); @@ -406,7 +406,7 @@ static inline rmr_mbuf_t* realloc_msg( rmr_mbuf_t* old_msg, int tr_len ) { nm->len = old_msg->len; // length of data in the payload nm->alloc_len = mlen; // length of allocated payload - nm->xaction = hdr->xid; // reference xaction + nm->xaction = &hdr->xid[0]; // reference xaction nm->state = old_msg->state; // fill in caller's state (likely the state of the last operation) nm->flags = old_msg->flags | MFL_ZEROCOPY; // this is a zerocopy sendable message memcpy( nm->payload, old_msg->payload, old_msg->len ); @@ -502,6 +502,7 @@ static inline rmr_mbuf_t* realloc_payload( rmr_mbuf_t* old_msg, int payload_len, if( DEBUG ) rmr_vlog( RMR_VL_DEBUG, "reallocate for payload increase. new message size: %d\n", (int) mlen ); if( (nm->tp_buf = (char *) malloc( sizeof( char ) * mlen )) == NULL ) { rmr_vlog( RMR_VL_CRIT, "rmr_realloc_payload: cannot get memory for zero copy buffer. bytes requested: %d\n", (int) mlen ); + free( nm ); return NULL; } -- 2.16.6