Fixed memory deallocation/cleanup on error returns

For the credentials structures, established the convention that any of
the Kerberos routines that mutate the credentials structures shall
free substructure before replacing it.

git-svn-id: svn://anonsvn.mit.edu/krb5/trunk@3320 dc483132-0cff-0310-8789-dd5450dbe970

7 files changed, 118 insertions, 108 deletions

diff --git a/src/lib/krb5/krb/copy_tick.c b/src/lib/krb5/krb/copy_tick.c
index 8871d1b08..d10fa3bb2 100644
--- a/src/lib/krb5/krb/copy_tick.c
+++ b/src/lib/krb5/krb/copy_tick.c
@@ -67,7 +67,7 @@ krb5_enc_tkt_part **partto;
 	    krb5_free_principal(tempto->client);
 	    krb5_free_keyblock(tempto->session);
 	    krb5_xfree(tempto);
-	    return retval;
+	    return ENOMEM;
 	}
 	memcpy((char *)tempto->transited.tr_contents.data,
 	       (char *)partfrom->transited.tr_contents.data,
@@ -111,8 +111,10 @@ krb5_ticket **pto;
 	return ENOMEM;
     *tempto = *from;
     retval = krb5_copy_principal(from->server, &tempto->server);
-    if (retval)
+    if (retval) {
+	krb5_xfree(tempto);
 	return retval;
+    }
     retval = krb5_copy_data(&from->enc_part.ciphertext, &scratch);
     if (retval) {
 	krb5_free_principal(tempto->server);
diff --git a/src/lib/krb5/krb/gc_2tgt.c b/src/lib/krb5/krb/gc_2tgt.c
index 2aca318bc..80b5bc9d8 100644
--- a/src/lib/krb5/krb/gc_2tgt.c
+++ b/src/lib/krb5/krb/gc_2tgt.c
@@ -119,72 +119,73 @@ register krb5_creds * cred;
     if (retval)
 	return retval;
 
-#undef cleanup
-#define cleanup() {\
-	memset((char *)dec_rep->enc_part2->session->contents, 0,\
-	      dec_rep->enc_part2->session->length);\
-		  krb5_free_kdc_rep(dec_rep); }
-
     if (dec_rep->msg_type != KRB5_TGS_REP) {
-	cleanup();
-	return KRB5KRB_AP_ERR_MSG_TYPE;
+	retval = KRB5KRB_AP_ERR_MSG_TYPE;
+	goto errout;
     }
     
     /* now it's decrypted and ready for prime time */
 
     if (!krb5_principal_compare(dec_rep->client, tgt->client)) {
-	cleanup();
-	return KRB5_KDCREP_MODIFIED;
+	retval = KRB5_KDCREP_MODIFIED;
+	goto errout;
     }
     /* put pieces into cred-> */
-    if (retval = krb5_copy_keyblock_contents(dec_rep->enc_part2->session,
-					     &cred->keyblock)) {
-	cleanup();
-	return retval;
+    if (cred->keyblock.contents) {
+	memset(&cred->keyblock.contents, 0, cred->keyblock.length);
+	krb5_xfree(cred->keyblock.contents);
     }
-    memset((char *)dec_rep->enc_part2->session->contents, 0,
-	  dec_rep->enc_part2->session->length);
-
-#undef cleanup
-#define cleanup() {\
-	memset((char *)cred->keyblock.contents, 0, cred->keyblock.length);\
-		  krb5_free_kdc_rep(dec_rep); }
+    if (retval = krb5_copy_keyblock_contents(dec_rep->enc_part2->session,
+					     &cred->keyblock))
+	goto errout;
 
     /* Should verify that the ticket is what we asked for. */
     cred->times = dec_rep->enc_part2->times;
     cred->ticket_flags = dec_rep->enc_part2->flags;
     cred->is_skey = TRUE;
-    if (dec_rep->enc_part2->caddrs) {
-	if (retval = krb5_copy_addresses(dec_rep->enc_part2->caddrs,
-					 &cred->addresses)) {
-	    cleanup();
-	    return retval;
-	}
-    } else {
+    if (cred->addresses)
+	krb5_free_addresses(cred->addresses);
+    if (dec_rep->enc_part2->caddrs)
+	retval = krb5_copy_addresses(dec_rep->enc_part2->caddrs,
+				     &cred->addresses);
+    else
 	/* no addresses in the list means we got what we had */
-	if (retval = krb5_copy_addresses(tgt->addresses,
-					 &cred->addresses)) {
-	    cleanup();
-	    return retval;
-	}
-    }
+	retval = krb5_copy_addresses(tgt->addresses, &cred->addresses);
+    if (retval)
+	    goto errout;
+    
+    if (cred->server)
+	krb5_free_principal(cred->server);
     if (retval = krb5_copy_principal(dec_rep->enc_part2->server,
-				     &cred->server)) {
-	cleanup();
-	return retval;
-    }
+				     &cred->server))
+	goto errout;
 
-    if (retval = encode_krb5_ticket(dec_rep->ticket, &scratch)) {
-	cleanup();
-	krb5_free_addresses(cred->addresses);
-	return retval;
-    }
+    if (retval = encode_krb5_ticket(dec_rep->ticket, &scratch))
+	goto errout;
 
     cred->ticket = *scratch;
     krb5_xfree(scratch);
 
+errout:
+    if (retval) {
+	if (cred->keyblock.contents) {
+	    memset((char *)cred->keyblock.contents, 0, cred->keyblock.length);
+	    krb5_xfree(cred->keyblock.contents);
+	    cred->keyblock.contents = 0;
+	}
+	if (cred->addresses) {
+	    krb5_free_addresses(cred->addresses);
+	    cred->addresses = 0;
+	}
+	if (cred->server) {
+	    krb5_free_principal(cred->server);
+	    cred->server = 0;
+	}
+    }
+    memset((char *)dec_rep->enc_part2->session->contents, 0,
+	   dec_rep->enc_part2->session->length);
     krb5_free_kdc_rep(dec_rep);
-    return 0;
+    return retval;
 }
 
 /*
diff --git a/src/lib/krb5/krb/gc_frm_kdc.c b/src/lib/krb5/krb/gc_frm_kdc.c
index 0110328c3..c825efa2f 100644
--- a/src/lib/krb5/krb/gc_frm_kdc.c
+++ b/src/lib/krb5/krb/gc_frm_kdc.c
@@ -73,7 +73,7 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 {
     krb5_creds tgt, tgtq;
     krb5_creds **ret_tgts = 0;
-    krb5_principal *tgs_list, *next_server;
+    krb5_principal *tgs_list = 0, *next_server;
     krb5_principal final_server;
     krb5_error_code retval;
     int nservers;
@@ -82,6 +82,9 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 
     /* in case we never get a TGT, zero the return */
     *tgts = 0;
+    
+    memset((char *)&tgtq, 0, sizeof(tgtq));
+    memset((char *)&tgt, 0, sizeof(tgt));
 
     /*
      * we know that the desired credentials aren't in the cache yet.
@@ -99,20 +102,14 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
      *  realm's KDC; so we use KRB5_TC_MATCH_SRV_NAMEONLY below)
      */
 
-    /*
-     * we're sharing some substructure here, which is dangerous.
-     * Be sure that if you muck with things here that tgtq.* doesn't share
-     * any substructure before you deallocate/clean up/whatever.
-     */
-    memset((char *)&tgtq, 0, sizeof(tgtq));
-    tgtq.client = cred->client;
-
+    if (retval = krb5_copy_principal(cred->client, &tgtq.client))
+	goto errout;
     if (retval = krb5_tgtname(krb5_princ_realm(cred->server),
 			      krb5_princ_realm(cred->client), &final_server))
-	return retval;
-    tgtq.server = final_server;
+	goto errout;
+    if (retval = krb5_copy_principal(final_server, &tgtq.server))
+	goto errout;
 
-    /* try to fetch it directly */
     retval = krb5_cc_retrieve_cred (ccache,
 				    KRB5_TC_MATCH_SRV_NAMEONLY,
 				    &tgtq,
@@ -120,16 +117,16 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 
     if (retval != 0) {
 	if (retval != KRB5_CC_NOTFOUND)
-	    goto out;
+	    goto errout;
 	/* don't have the right TGT in the cred cache.  Time to iterate
 	   across realms to get the right TGT. */
 
 	/* get a list of realms to consult */
-	retval = krb5_walk_realm_tree(krb5_princ_realm(cred->client),
-				      krb5_princ_realm(cred->server),
-				      &tgs_list, KRB5_REALM_BRANCH_CHAR);
-	if (retval)
-	    goto out;
+	if (retval = krb5_walk_realm_tree(krb5_princ_realm(cred->client),
+					  krb5_princ_realm(cred->server),
+					  &tgs_list, KRB5_REALM_BRANCH_CHAR))
+	    goto errout;
+
 	/* walk the list BACKWARDS until we find a cached
 	   TGT, then move forward obtaining TGTs until we get the last
 	   TGT needed */
@@ -140,16 +137,16 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 
 	/* next_server now points to the last TGT */
 	for (; next_server >= tgs_list; next_server--) {
-	    tgtq.server = *next_server;
+	    krb5_free_principal(tgtq.server);
+	    if (retval = krb5_copy_principal(*next_server, &tgt.server))
+		goto errout;
 	    retval = krb5_cc_retrieve_cred (ccache,
 					    KRB5_TC_MATCH_SRV_NAMEONLY,
 					    &tgtq,
 					    &tgt);
 	    if (retval) {
-		if (retval != KRB5_CC_NOTFOUND) {
-		    krb5_free_realm_tree(tgs_list);
-		    goto out;
-		}
+		if (retval != KRB5_CC_NOTFOUND)
+		    goto errout;
 		continue;
 	    }
 	    next_server++;
@@ -158,38 +155,38 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 	if (next_server < tgs_list) {
 	    /* didn't find any */
 	    retval = KRB5_NO_TKT_IN_RLM;
-	    krb5_free_realm_tree(tgs_list);
-	    goto out;
+	    goto errout;
 	}
 	/* allocate storage for TGT pointers. */
 	ret_tgts = (krb5_creds **)calloc(nservers+1, sizeof(krb5_creds));
 	if (!ret_tgts) {
 	    retval = ENOMEM;
-	    krb5_free_realm_tree(tgs_list);
-	    goto out;
+	    goto errout;
 	}
 	*tgts = ret_tgts;
 	for (nservers = 0; *next_server; next_server++, nservers++) {
-
 	    krb5_data *tmpdata;
 
 	    if (!valid_keytype(tgt.keyblock.keytype)) {
 		retval = KRB5_PROG_KEYTYPE_NOSUPP;
-		krb5_free_realm_tree(tgs_list);
-		goto out;
+		goto errout;
 	    }
 	    /* now get the TGTs */
+	    krb5_free_cred_contents(&tgtq);
 	    memset((char *)&tgtq, 0, sizeof(tgtq));
 	    tgtq.times = tgt.times;
-	    tgtq.client = tgt.client;
+	    if (retval = krb5_copy_principal(tgt.client, &tgtq.client))
+		goto errout;
 
 	    /* ask each realm for a tgt to the end */
-	    if (retval = krb5_copy_data(krb5_princ_realm(*next_server), &tmpdata)) {
-		krb5_free_realm_tree(tgs_list);
-		goto out;
+	    if (retval = krb5_copy_data(krb5_princ_realm(*next_server),
+					&tmpdata)) {
+		goto errout;
 	    }
+	    free(krb5_princ_realm(final_server)->data);
 	    krb5_princ_set_realm(final_server, tmpdata);
-	    tgtq.server = final_server;
+	    if (retval = krb5_copy_principal(final_server, &tgtq.server))
+		goto errout;
 
 	    tgtq.is_skey = FALSE;
 	    tgtq.ticket_flags = tgt.ticket_flags;
@@ -199,10 +196,9 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 					       flags2options(tgtq.ticket_flags),
 					       etype,
 					       krb5_kdc_req_sumtype,
-					       &tgtq)) {
-		krb5_free_realm_tree(tgs_list);
-		goto out;
-	    }
+					       &tgtq))
+		goto errout;
+
 	    /* make sure the returned ticket is somewhere in the remaining
 	       list, but we can tolerate different expected issuing realms */
 	    while (*++next_server &&
@@ -210,28 +206,21 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 					   &(tgtq.server[1])));
 	    if (!next_server) {
 		/* what we got back wasn't in the list! */
-		krb5_free_realm_tree(tgs_list);
 		retval = KRB5_KDCREP_MODIFIED;
-		goto out;
+		goto errout;
 	    }
 							   
 	    /* save tgt in return array */
-	    if (retval = krb5_copy_creds(&tgtq, &ret_tgts[nservers])) {
-		krb5_free_realm_tree(tgs_list);
-		goto out;
-	    }
+	    if (retval = krb5_copy_creds(&tgtq, &ret_tgts[nservers]))
+		goto errout;
 	    tgt = *ret_tgts[nservers];
 	    returning_tgt = 1;		/* don't free it below... */
-	    tgtq.client = 0;
-	    tgtq.server = 0;
-	    krb5_free_cred_contents(&tgtq);
 	}
-	krb5_free_realm_tree(tgs_list);
     }
     /* got/finally have tgt! */
     if (!valid_keytype(tgt.keyblock.keytype)) {
 	retval = KRB5_PROG_KEYTYPE_NOSUPP;
-	goto out;
+	goto errout;
     }
     etype = krb5_keytype_array[tgt.keyblock.keytype]->system->proto_enctype;
 
@@ -246,10 +235,13 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 				       etype,
 				       krb5_kdc_req_sumtype,
 				       cred);
-    
+errout:
     if (!returning_tgt)
 	krb5_free_cred_contents(&tgt);
-out:
-    krb5_free_principal(final_server);
+    if (final_server)
+	    krb5_free_principal(final_server);
+    krb5_free_cred_contents(&tgtq);
+    if (tgs_list)
+	krb5_free_realm_tree(tgs_list);
     return retval;
 }
diff --git a/src/lib/krb5/krb/gc_via_tgt.c b/src/lib/krb5/krb/gc_via_tgt.c
index d6ef9119b..23dc668f3 100644
--- a/src/lib/krb5/krb/gc_via_tgt.c
+++ b/src/lib/krb5/krb/gc_via_tgt.c
@@ -143,6 +143,10 @@ OLDDECLARG(krb5_creds *, cred)
 	return KRB5_KDCREP_MODIFIED;
     }
     /* put pieces into cred-> */
+    if (cred->keyblock.contents) {
+	memset(&cred->keyblock.contents, 0, cred->keyblock.length);
+	krb5_xfree(cred->keyblock.contents);
+    }
     if (retval = krb5_copy_keyblock_contents(dec_rep->enc_part2->session,
 					     &cred->keyblock)) {
 	cleanup();
@@ -188,6 +192,9 @@ OLDDECLARG(krb5_creds *, cred)
 
     cred->ticket_flags = dec_rep->enc_part2->flags;
     cred->is_skey = FALSE;
+    if (cred->addresses) {
+	krb5_free_addresses(cred->addresses);
+    }
     if (dec_rep->enc_part2->caddrs) {
 	if (retval = krb5_copy_addresses(dec_rep->enc_part2->caddrs,
 					 &cred->addresses)) {
@@ -205,7 +212,8 @@ OLDDECLARG(krb5_creds *, cred)
     /*
      * Free cred->server before overwriting it.
      */
-    krb5_free_principal(cred->server);
+    if (cred->server)
+	krb5_free_principal(cred->server);
     if (retval = krb5_copy_principal(dec_rep->enc_part2->server,
 				     &cred->server)) {
 	cleanup();
diff --git a/src/lib/krb5/krb/get_in_tkt.c b/src/lib/krb5/krb/get_in_tkt.c
index 320a3b208..28ff4739e 100644
--- a/src/lib/krb5/krb/get_in_tkt.c
+++ b/src/lib/krb5/krb/get_in_tkt.c
@@ -320,6 +320,7 @@ OLDDECLARG(krb5_kdc_rep **, ret_as_reply)
     if (retval = krb5_copy_addresses(as_reply->enc_part2->caddrs,
 				     &creds->addresses)) {
 	cleanup_key();
+	krb5_free_kdc_rep(as_reply);
 	return retval;
     }
     creds->second_ticket.length = 0;
@@ -330,6 +331,7 @@ OLDDECLARG(krb5_kdc_rep **, ret_as_reply)
 	krb5_free_kdc_rep(as_reply);
 	krb5_free_addresses(creds->addresses);
 	cleanup_key();
+	krb5_free_kdc_rep(as_reply);
 	return retval;
     }	
     creds->ticket = *packet;
@@ -342,6 +344,7 @@ OLDDECLARG(krb5_kdc_rep **, ret_as_reply)
 	krb5_xfree(creds->ticket.data);
 	krb5_free_addresses(creds->addresses);
 	cleanup_key();
+	krb5_free_kdc_rep(as_reply);
 	return retval;
     }
     if (ret_as_reply)
diff --git a/src/lib/krb5/krb/mk_req.c b/src/lib/krb5/krb/mk_req.c
index d9afd1cbf..252ec976d 100644
--- a/src/lib/krb5/krb/mk_req.c
+++ b/src/lib/krb5/krb/mk_req.c
@@ -69,9 +69,10 @@ krb5_data *outbuf;
     /* obtain ticket & session key */
 
     memset((char *)&creds, 0, sizeof(creds));
-    creds.server = (krb5_principal) server;
+    if (retval = krb5_copy_principal(server, &creds.server))
+	goto errout;
     if (retval = krb5_cc_get_principal(ccache, &creds.client))
-	return(retval);
+	goto errout;
     /* creds.times.endtime = 0; -- memset 0 takes care of this
      				   zero means "as long as possible" */
     /* creds.keyblock.keytype = 0; -- as well as this.
@@ -81,7 +82,7 @@ krb5_data *outbuf;
     if (retval = krb5_get_credentials(krb5_kdc_default_options,
 				      ccache,
 				      &creds))
-	return(retval);
+	goto errout;
 
     retval = krb5_mk_req_extended(ap_req_options,
 				  checksum,
@@ -92,9 +93,8 @@ krb5_data *outbuf;
 				  &creds,
 				  0, 	/* We don't need the authenticator */
 				  outbuf);
-    /* creds.server and the rest of the creds.* fields are filled in
-       by the ccache fetch or the kdc fetch, so we should allow it to be
-       freed */
+
+errout:
     krb5_free_cred_contents(&creds);
     return retval;
 }
diff --git a/src/lib/krb5/krb/rd_safe.c b/src/lib/krb5/krb/rd_safe.c
index 8e6cd992d..b884e1660 100644
--- a/src/lib/krb5/krb/rd_safe.c
+++ b/src/lib/krb5/krb/rd_safe.c
@@ -78,11 +78,15 @@ krb5_data *outbuf;
 
 #define cleanup() krb5_free_safe(message)
 
-    if (!valid_cksumtype(message->checksum->checksum_type))
+    if (!valid_cksumtype(message->checksum->checksum_type)) {
+	cleanup();
 	return KRB5_PROG_SUMTYPE_NOSUPP;
+    }
     if (!is_coll_proof_cksum(message->checksum->checksum_type) ||
-	!is_keyed_cksum(message->checksum->checksum_type))
+	!is_keyed_cksum(message->checksum->checksum_type)) {
+	cleanup();
 	return KRB5KRB_AP_ERR_INAPP_CKSUM;
+    }
 
     if (!(safe_flags & KRB5_SAFE_NOTIME)) {
 	krb5_donot_replay replay;