Merge pull request #79 from painless-security/jennifer/memory_leaks

Clean up several memory leaks detected by valgrind

diff --git a/gsscon/gsscon_active.c b/gsscon/gsscon_active.c
index 1227c2b..e730b55 100755 (executable)
--- a/gsscon/gsscon_active.c
+++ b/gsscon/gsscon_active.c
@@ -73,7 +73,6 @@ int gsscon_connect (const char *inHost, unsigned int inPort, const char *inServi
   struct sockaddr_in saddr;
   char *port=NULL;
   gss_name_t serviceName = NULL;
-  gss_name_t clientName = NULL;
   gss_cred_id_t clientCredentials = GSS_C_NO_CREDENTIAL;
   gss_ctx_id_t gssContext = GSS_C_NO_CONTEXT;
   OM_uint32 actualFlags = 0;
@@ -83,6 +82,7 @@ int gsscon_connect (const char *inHost, unsigned int inPort, const char *inServi
   gss_buffer_desc nameBuffer;
   gss_buffer_t inputTokenPtr = GSS_C_NO_BUFFER;
   char *name;
+  int len = 0;
 
   if (!inServiceName) { err = EINVAL; }
   if (!outGSSContext) { err = EINVAL; }
@@ -128,7 +128,7 @@ int gsscon_connect (const char *inHost, unsigned int inPort, const char *inServi
   if (fd >= 0) { close (fd); }
 
   if (!err) {
-    majorStatus = gss_acquire_cred (&minorStatus, clientName, GSS_C_INDEFINITE, GSS_C_NO_OID_SET, 
+    majorStatus = gss_acquire_cred (&minorStatus, GSS_C_NO_NAME, GSS_C_INDEFINITE, GSS_C_NO_OID_SET,
                                     GSS_C_INITIATE, &clientCredentials, NULL, NULL); 
     if (majorStatus != GSS_S_COMPLETE) { 
       gsscon_print_gss_errors ("gss_acquire_cred", majorStatus, minorStatus);
@@ -185,16 +185,27 @@ int gsscon_connect (const char *inHost, unsigned int inPort, const char *inServi
    */
     
   if (!err) {
-    nameBuffer.length = asprintf(&name, "%s@%s", inServiceName, inHost);
-    nameBuffer.value = name;
+    len = asprintf(&name, "%s@%s", inServiceName, inHost);
+    if (len < 0) {
+      /* asprintf failed, pick an error to return... */
+      err = GSS_S_BAD_NAME;
+    } else {
+      nameBuffer.length = (size_t) len;
+      nameBuffer.value = name;
 
-    majorStatus = gss_import_name (&minorStatus, &nameBuffer, (gss_OID) GSS_C_NT_HOSTBASED_SERVICE, &serviceName); 
-    if (majorStatus != GSS_S_COMPLETE) { 
-      gsscon_print_gss_errors ("gss_import_name(inServiceName)", majorStatus, minorStatus);
-      err = minorStatus ? minorStatus : majorStatus; 
+      majorStatus = gss_import_name (&minorStatus, &nameBuffer, (gss_OID) GSS_C_NT_HOSTBASED_SERVICE, &serviceName);
+      if (majorStatus != GSS_S_COMPLETE) {
+        gsscon_print_gss_errors ("gss_import_name(inServiceName)", majorStatus, minorStatus);
+        err = minorStatus ? minorStatus : majorStatus;
+      }
+
+      /* free the input name and null pointers to avoid reuse */
+      free(name);
+      name = NULL;
+      nameBuffer.value = NULL;
     }
   }
-    
+
   /* 
    * The main authentication loop:
    *
@@ -271,7 +282,6 @@ int gsscon_connect (const char *inHost, unsigned int inPort, const char *inServi
 
   if (inputTokenBuffer) { free (inputTokenBuffer); }
   if (serviceName     ) { gss_release_name (&minorStatus, &serviceName); }
-  if (clientName      ) { gss_release_name (&minorStatus, &clientName); }
   if (ai_head         ) { freeaddrinfo(ai_head); }
 
   if (clientCredentials != GSS_C_NO_CREDENTIAL) { 

diff --git a/tr/tr_trp.c b/tr/tr_trp.c
index 9f9c558..6cf2e46 100644 (file)
--- a/tr/tr_trp.c
+++ b/tr/tr_trp.c
@@ -187,10 +187,12 @@ static void tr_trps_event_cb(int listener, short event, void *arg)
     name = talloc_asprintf(tmp_ctx, "trustrouter@%s", trps->hostname);
     if (name == NULL)
       goto cleanup;
-    gssname=tr_new_name(name); /* name cleaned up with tmp_ctx */
+    gssname=tr_new_name(name); /* name cleaned up with tmp_ctx but need to handl gssname ourselves */
 
-    conn=trp_connection_accept(tmp_ctx, listener, gssname);
-    if (conn!=NULL) {
+    conn=trp_connection_accept(tmp_ctx, listener, gssname); /* steals gssname unless it fails */
+    if (conn == NULL) {
+      tr_free_name(gssname);
+    } else {
       /* need to monitor this fd and trigger events when read becomes possible */
       thread_data=talloc(conn, struct trps_thread_data);
       if (thread_data==NULL) {

diff --git a/trp/trp_rtable.c b/trp/trp_rtable.c
index 2926309..9b915a2 100644 (file)
--- a/trp/trp_rtable.c
+++ b/trp/trp_rtable.c
@@ -371,7 +371,9 @@ TRP_ROUTE **trp_rtable_get_comm_entries(TRP_RTABLE *rtbl, TR_NAME *comm, size_t
 
 /* Get all entries in an comm/realm. Returns an array of pointers in NULL talloc context.
  * Caller must free this list with talloc_free, but must not free the entries in the
- * list.. */
+ * list.
+ *
+ * If *n_out is 0, then no memory is allocated and NULL is returned. */
 TRP_ROUTE **trp_rtable_get_realm_entries(TRP_RTABLE *rtbl, TR_NAME *comm, TR_NAME *realm, size_t *n_out)
 {
   size_t ii=0;
@@ -380,16 +382,23 @@ TRP_ROUTE **trp_rtable_get_realm_entries(TRP_RTABLE *rtbl, TR_NAME *comm, TR_NAM
 
   tr_debug("trp_rtable_get_realm_entries: entered.");
   peer=trp_rtable_get_comm_realm_peers(rtbl, comm, realm, n_out);
+  if ((peer == NULL) || (*n_out == 0)) {
+    *n_out = 0; /* May be redundant. That's ok, compilers are smart. */
+    goto cleanup;
+  }
+
   ret=talloc_array(NULL, TRP_ROUTE *, *n_out);
   if (ret==NULL) {
     tr_crit("trp_rtable_get_realm_entries: could not allocate return array.");
-    talloc_free(peer);
     n_out=0;
-    return NULL;
+    goto cleanup;
   }
   for (ii=0; ii<*n_out; ii++)
     ret[ii]=trp_rtable_get_entry(rtbl, comm, realm, peer[ii]);
-  talloc_free(peer);
+
+cleanup:
+  if (peer)
+    talloc_free(peer);
   return ret;
 }
 

diff --git a/trp/trp_upd.c b/trp/trp_upd.c
index e1057f1..9c520e3 100644 (file)
--- a/trp/trp_upd.c
+++ b/trp/trp_upd.c
@@ -260,19 +260,31 @@ TR_NAME *trp_inforec_dup_next_hop(TRP_INFOREC *rec)
   return tr_dup_name(trp_inforec_get_next_hop(rec));
 }
 
+/**
+ * Set the next hop for the inforec
+ *
+ * Returns TRP_SUCCESS if it set the next hop value for the inforec.
+ * Returns TRP_UNSUPPORTED if the inforec does not have a next hop record but
+ * otherwise nothing went wrong.
+ * Returns TRP_ERROR or another error if there was a failure.
+ *
+ * @param rec
+ * @param next_hop
+ * @return TRP_SUCCESS if the value was set, TRP_UNSUPPORTED if the inforec does not support next hop, or an error code on failure
+ */
 TRP_RC trp_inforec_set_next_hop(TRP_INFOREC *rec, TR_NAME *next_hop)
 {
+  /* Any inforec types that support next_hop should set it here. */
   switch (rec->type) {
   case TRP_INFOREC_TYPE_ROUTE:
     if (rec->data->route==NULL)
       return TRP_ERROR;
     rec->data->route->next_hop=next_hop;
     break;
-  case TRP_INFOREC_TYPE_COMMUNITY:
-    /* next hop not used for community records */
-    break;
+
   default:
-    break;
+    /* next hop not used for other records */
+    return TRP_UNSUPPORTED;
   }
   return TRP_SUCCESS;
 }
@@ -770,11 +782,22 @@ void trp_upd_set_next_hop(TRP_UPD *upd, const char *hostname, unsigned int port)
 {
   TRP_INFOREC *rec=NULL;
   TR_NAME *cpy=NULL;
-  
+
   for (rec=trp_upd_get_inforec(upd); rec!=NULL; rec=trp_inforec_get_next(rec)) {
-    if (trp_inforec_set_next_hop(rec, cpy=tr_new_name(hostname)) != TRP_SUCCESS) {
-      tr_err("trp_upd_set_next_hop: error setting next hop.");
-      tr_free_name(cpy);
+    switch (trp_inforec_set_next_hop(rec, cpy=tr_new_name(hostname))) {
+      case TRP_SUCCESS:
+        /* Success, the TR_NAME in cpy is now stored with the inforec */
+        break;
+
+      case TRP_UNSUPPORTED:
+        /* No error, but the inforec does not accept a next_hop. Free our copy. */
+        tr_free_name(cpy);
+        break;
+
+      default:
+        tr_err("trp_upd_set_next_hop: error setting next hop.");
+        tr_free_name(cpy);
+        break;
     }
   }
 }