Patch from Scott McKellar:

1. Added the const qualifier in various places.

2. Eliminated some unnecessary calls to strlen(), where they were
used merely to determine whether a string was empty.

3. Ensured that all members of a new transport_message are
explicitly populated.

4. Plugged a memory leak in the case where strdup() fails.

5. Eliminated some unhelpful casts of malloc'd pointers.

6. Added some protective tests for NULL pointer parameters.

7. In several spots I replaced numeric literals with character
literals, both to make the code more readable and to avoid a needless
dependence on ASCII.

8. Rewrote the jid_get_resource function to avoid repeatedly
overwriting the same buffer.

git-svn-id: svn://svn.open-ils.org/OpenSRF/trunk@1194 9efc2488-bf62-4759-914b-345cdb29e865

diff --git a/include/opensrf/transport_message.h b/include/opensrf/transport_message.h
index ddd8f39..6cf3a49 100644 (file)
--- a/include/opensrf/transport_message.h
+++ b/include/opensrf/transport_message.h
@@ -28,7 +28,7 @@ struct transport_message_struct {
        char* router_to;
        char* router_class;
        char* router_command;
-   char* osrf_xid;
+       char* osrf_xid;
        int is_error;
        char* error_type;
        int error_code;
@@ -42,16 +42,17 @@ typedef struct transport_message_struct transport_message;
 // within this method.
 // Returns NULL on error
 // ---------------------------------------------------------------------------------
-transport_message* message_init( char* body, char* subject, 
-               char* thread, char* recipient, char* sender );
+transport_message* message_init( const char* body, const char* subject, 
+               const char* thread, const char* recipient, const char* sender );
 
 transport_message* new_message_from_xml( const char* msg_xml );
 
 
-void message_set_router_info( transport_message* msg, char* router_from,
-               char* router_to, char* router_class, char* router_command, int broadcast_enabled );
+void message_set_router_info( transport_message* msg, const char* router_from,
+               const char* router_to, const char* router_class, const char* router_command,
+               int broadcast_enabled );
 
-void message_set_osrf_xid( transport_message* msg, char* osrf_xid );
+void message_set_osrf_xid( transport_message* msg, const char* osrf_xid );
 
 // ---------------------------------------------------------------------------------
 // Formats the Jabber message as XML for encoding. 
@@ -93,7 +94,7 @@ void jid_get_resource( const char* jid, char buf[], int size );
 /** Puts the domain portion of the given jid into the pre-allocated buffer */
 void jid_get_domain( const char* jid, char buf[], int size );
 
-void set_msg_error( transport_message*, char* error_type, int error_code);
+void set_msg_error( transport_message*, const char* error_type, int error_code);
 
 
 #endif

diff --git a/src/libopensrf/transport_message.c b/src/libopensrf/transport_message.c
index 80b1ea9..c66df27 100644 (file)
--- a/src/libopensrf/transport_message.c
+++ b/src/libopensrf/transport_message.c
@@ -4,17 +4,16 @@
 // ---------------------------------------------------------------------------------
 // Allocates and initializes a new transport_message
 // ---------------------------------------------------------------------------------
-transport_message* message_init( char* body, 
-               char* subject, char* thread, char* recipient, char* sender ) {
+transport_message* message_init( const char* body, const char* subject,
+               const char* thread, const char* recipient, const char* sender ) {
 
-       transport_message* msg = 
-               (transport_message*) safe_malloc( sizeof(transport_message) );
+       transport_message* msg = safe_malloc( sizeof(transport_message) );
 
-       if( body                                        == NULL ) { body                        = ""; }
+       if( body                                == NULL ) { body                = ""; }
        if( thread                              == NULL ) { thread              = ""; }
        if( subject                             == NULL ) { subject             = ""; }
        if( sender                              == NULL ) { sender              = ""; }
-       if( recipient                   ==      NULL ) { recipient      = ""; }
+       if( recipient                   == NULL ) { recipient   = ""; }
 
        msg->body                               = strdup(body);
        msg->thread                             = strdup(thread);
@@ -27,20 +26,52 @@ transport_message* message_init( char* body,
                        msg->sender             == NULL ) {
 
                osrfLogError(OSRF_LOG_MARK,  "message_init(): Out of Memory" );
+               free( msg->body );
+               free( msg->thread );
+               free( msg->subject );
+               free( msg->recipient );
+               free( msg->sender );
+               free( msg );
                return NULL;
        }
 
+       msg->router_from    = NULL;
+       msg->router_to      = NULL;
+       msg->router_class   = NULL;
+       msg->router_command = NULL;
+       msg->osrf_xid       = NULL;
+       msg->is_error       = 0;
+       msg->error_type     = NULL;
+       msg->error_code     = 0;
+       msg->broadcast      = 0;
+       msg->msg_xml        = NULL;
+
        return msg;
 }
 
 
 transport_message* new_message_from_xml( const char* msg_xml ) {
 
-       if( msg_xml == NULL || strlen(msg_xml) < 1 )
+       if( msg_xml == NULL || *msg_xml == '\0' )
                return NULL;
 
-       transport_message* new_msg = 
-               (transport_message*) safe_malloc( sizeof(transport_message) );
+       transport_message* new_msg = safe_malloc( sizeof(transport_message) );
+
+       new_msg->body           = NULL;
+       new_msg->subject        = NULL;
+       new_msg->thread         = NULL;
+       new_msg->recipient      = NULL;
+       new_msg->sender         = NULL;
+       new_msg->router_from    = NULL;
+       new_msg->router_to      = NULL;
+       new_msg->router_class   = NULL;
+       new_msg->router_command = NULL;
+       new_msg->osrf_xid       = NULL;
+       new_msg->is_error       = 0;
+       new_msg->error_type     = NULL;
+       new_msg->error_code     = 0;
+       new_msg->broadcast      = 0;
+       new_msg->msg_xml        = NULL;
 
        xmlKeepBlanksDefault(0);
        xmlDocPtr msg_doc = xmlReadDoc( BAD_CAST msg_xml, NULL, NULL, 0 );
@@ -54,7 +85,7 @@ transport_message* new_message_from_xml( const char* msg_xml ) {
        xmlChar* router_to      = xmlGetProp( root, BAD_CAST "router_to" );
        xmlChar* router_class= xmlGetProp( root, BAD_CAST "router_class" );
        xmlChar* broadcast      = xmlGetProp( root, BAD_CAST "broadcast" );
-   xmlChar* osrf_xid    = xmlGetProp( root, BAD_CAST "osrf_xid" );
+       xmlChar* osrf_xid    = xmlGetProp( root, BAD_CAST "osrf_xid" );
 
    if( osrf_xid ) {
       message_set_osrf_xid( new_msg, (char*) osrf_xid);
@@ -62,40 +93,40 @@ transport_message* new_message_from_xml( const char* msg_xml ) {
    }
 
        if( router_from ) {
-               new_msg->sender         = strdup((char*)router_from);
+               new_msg->sender         = strdup((const char*)router_from);
        } else {
                if( sender ) {
-                       new_msg->sender         = strdup((char*)sender);
+                       new_msg->sender         = strdup((const char*)sender);
                        xmlFree(sender);
                }
        }
 
        if( recipient ) {
-               new_msg->recipient      = strdup((char*)recipient);
+               new_msg->recipient      = strdup((const char*)recipient);
                xmlFree(recipient);
        }
        if(subject){
-               new_msg->subject                = strdup((char*)subject);
+               new_msg->subject                = strdup((const char*)subject);
                xmlFree(subject);
        }
        if(thread) {
-               new_msg->thread         = strdup((char*)thread);
+               new_msg->thread         = strdup((const char*)thread);
                xmlFree(thread);
        }
        if(router_from) {
-               new_msg->router_from    = strdup((char*)router_from);
+               new_msg->router_from    = strdup((const char*)router_from);
                xmlFree(router_from);
        }
        if(router_to) {
-               new_msg->router_to      = strdup((char*)router_to);
+               new_msg->router_to      = strdup((const char*)router_to);
                xmlFree(router_to);
        }
        if(router_class) {
-               new_msg->router_class = strdup((char*)router_class);
+               new_msg->router_class = strdup((const char*)router_class);
                xmlFree(router_class);
        }
        if(broadcast) {
-               if(strcmp((char*) broadcast,"0") )
+               if(strcmp((const char*) broadcast,"0") )
                        new_msg->broadcast      = 1;
                xmlFree(broadcast);
        }
@@ -103,19 +134,19 @@ transport_message* new_message_from_xml( const char* msg_xml ) {
        xmlNodePtr search_node = root->children;
        while( search_node != NULL ) {
 
-               if( ! strcmp( (char*) search_node->name, "thread" ) ) {
+               if( ! strcmp( (const char*) search_node->name, "thread" ) ) {
                        if( search_node->children && search_node->children->content ) 
-                               new_msg->thread = strdup( (char*) search_node->children->content );
+                               new_msg->thread = strdup( (const char*) search_node->children->content );
                }
 
-               if( ! strcmp( (char*) search_node->name, "subject" ) ) {
+               if( ! strcmp( (const char*) search_node->name, "subject" ) ) {
                        if( search_node->children && search_node->children->content )
-                               new_msg->subject = strdup( (char*) search_node->children->content );
+                               new_msg->subject = strdup( (const char*) search_node->children->content );
                }
 
-               if( ! strcmp( (char*) search_node->name, "body" ) ) {
+               if( ! strcmp( (const char*) search_node->name, "body" ) ) {
                        if( search_node->children && search_node->children->content )
-                               new_msg->body = strdup((char*) search_node->children->content );
+                               new_msg->body = strdup((const char*) search_node->children->content );
                }
 
                search_node = search_node->next;
@@ -135,16 +166,19 @@ transport_message* new_message_from_xml( const char* msg_xml ) {
        return new_msg;
 }
 
-void message_set_osrf_xid( transport_message* msg, char* osrf_xid ) {
+void message_set_osrf_xid( transport_message* msg, const char* osrf_xid ) {
    if(!msg) return;
    if( osrf_xid )
       msg->osrf_xid = strdup(osrf_xid);
    else msg->osrf_xid = strdup("");
 }
 
-void message_set_router_info( transport_message* msg, char* router_from,
-               char* router_to, char* router_class, char* router_command, int broadcast_enabled ) {
+void message_set_router_info( transport_message* msg, const char* router_from,
+               const char* router_to, const char* router_class, const char* router_command,
+               int broadcast_enabled ) {
 
+       if( !msg ) return;
+       
        if(router_from)
                msg->router_from                = strdup(router_from);
        else
@@ -178,8 +212,9 @@ void message_set_router_info( transport_message* msg, char* router_from,
 
 /* encodes the message for traversal */
 int message_prepare_xml( transport_message* msg ) {
-       if( msg->msg_xml != NULL ) { return 1; }
-       msg->msg_xml = message_to_xml( msg );
+       if( !msg ) return 0;
+       if( msg->msg_xml == NULL )
+               msg->msg_xml = message_to_xml( msg );
        return 1;
 }
 
@@ -199,7 +234,7 @@ int message_free( transport_message* msg ){
        free(msg->router_to);
        free(msg->router_class);
        free(msg->router_command);
-   free(msg->osrf_xid);
+       free(msg->osrf_xid);
        if( msg->error_type != NULL ) free(msg->error_type);
        if( msg->msg_xml != NULL ) free(msg->msg_xml);
        free(msg);
@@ -227,7 +262,7 @@ char* message_to_xml( const transport_message* msg ) {
 
        if( ! msg ) { 
                osrfLogWarning(OSRF_LOG_MARK,  "Passing NULL message to message_to_xml()"); 
-               return 0; 
+               return NULL; 
        }
 
        doc = xmlReadDoc( BAD_CAST "<message/>", NULL, NULL, XML_PARSE_NSCLEAN );
@@ -260,21 +295,21 @@ char* message_to_xml( const transport_message* msg ) {
        char* subject                   = msg->subject;
        char* thread                    = msg->thread; 
 
-       if( thread && strlen(thread) > 0 ) {
+       if( thread && *thread ) {
                thread_node = xmlNewChild(message_node, NULL, (xmlChar*) "thread", NULL );
                xmlNodePtr txt = xmlNewText((xmlChar*) thread);
                xmlAddChild(thread_node, txt);
                xmlAddChild(message_node, thread_node); 
        }
 
-       if( subject && strlen(subject) > 0 ) {
+       if( subject && *subject ) {
                subject_node = xmlNewChild(message_node, NULL, (xmlChar*) "subject", NULL );
                xmlNodePtr txt = xmlNewText((xmlChar*) subject);
                xmlAddChild(subject_node, txt);
                xmlAddChild( message_node, subject_node ); 
        }
 
-       if( body && strlen(body) > 0 ) {
+       if( body && *body ) {
                body_node = xmlNewChild(message_node, NULL, (xmlChar*) "body", NULL);
                xmlNodePtr txt = xmlNewText((xmlChar*) body);
                xmlAddChild(body_node, txt);
@@ -283,7 +318,7 @@ char* message_to_xml( const transport_message* msg ) {
 
        xmlBufferPtr xmlbuf = xmlBufferCreate();
        xmlNodeDump( xmlbuf, doc, xmlDocGetRootElement(doc), 0, 0);
-       char* xml = strdup((char*) (xmlBufferContent(xmlbuf)));
+       char* xml = strdup((const char*) (xmlBufferContent(xmlbuf)));
        xmlBufferFree(xmlbuf);
        xmlFreeDoc( doc );               
        xmlCleanupParser();
@@ -294,16 +329,16 @@ char* message_to_xml( const transport_message* msg ) {
 
 void jid_get_username( const char* jid, char buf[], int size ) {
 
-       if( jid == NULL ) { return; }
+       if( jid == NULL || buf == NULL || size <= 0 ) { return; }
 
        /* find the @ and return whatever is in front of it */
        int len = strlen( jid );
        int i;
        for( i = 0; i != len; i++ ) {
-               if( jid[i] == 64 ) { /*ascii @*/
+               if( jid[i] == '@' ) {
                        if(i > size)  i = size;
-                       strncpy( buf, jid, i );
-                       buf[i] = '\0'; // strncpy doesn't provide the nul
+                       memcpy( buf, jid, i );
+                       buf[i] = '\0';
                        return;
                }
        }
@@ -311,17 +346,19 @@ void jid_get_username( const char* jid, char buf[], int size ) {
 
 
 void jid_get_resource( const char* jid, char buf[], int size)  {
-       if( jid == NULL ) { return; }
-       int len = strlen( jid );
-       int i;
-       for( i = 0; i!= len; i++ ) {
-               if( jid[i] == 47 ) { /* ascii / */
-                       const char* start = jid + i + 1; /* right after the '/' */
-                       int rlen = len - (i+1);
-                       if(rlen > size) rlen = size;
-                       strncpy( buf, start, rlen );
-                       buf[rlen] = '\0'; // strncpy doesn't provide the nul
-               }
+       if( jid == NULL || buf == NULL || size <= 0 ) { return; }
+
+       // Find the last slash, if any
+       
+       const char* start = strrchr( jid, '/' );
+       if( start ) {
+
+               // Copy the text beyond the slash, up to a maximum size
+
+               size_t len = strlen( ++start );
+               if( len > size ) len = size;
+               memcpy( buf, start, len );
+               buf[ len ] = '\0';
        }
 }
 
@@ -335,9 +372,9 @@ void jid_get_domain( const char* jid, char buf[], int size ) {
        int index2 = 0;
 
        for( i = 0; i!= len; i++ ) {
-               if(jid[i] == 64) /* ascii @ */
+               if(jid[i] == '@')
                        index1 = i + 1;
-               else if(jid[i] == 47 && index1 != 0) /* ascii / */
+               else if(jid[i] == '/' && index1 != 0)
                        index2 = i;
        }
 
@@ -349,9 +386,11 @@ void jid_get_domain( const char* jid, char buf[], int size ) {
        }
 }
 
-void set_msg_error( transport_message* msg, char* type, int err_code ) {
+void set_msg_error( transport_message* msg, const char* type, int err_code ) {
 
-       if( type != NULL && strlen( type ) > 0 ) {
+       if( !msg ) return;
+       
+       if( type != NULL && *type ) {
                msg->error_type = safe_malloc( strlen(type)+1); 
                strcpy( msg->error_type, type );
                msg->error_code = err_code;