(krb5_change_password):

Reorganized code so that krb5_change_password actually frees
everything it allocated on error.  Also fixed some memory 
leaks which happened even without an error occurring.

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

1 files changed, 284 insertions, 248 deletions

diff --git a/src/lib/krb5/os/changepw.c b/src/lib/krb5/os/changepw.c
index 779fc89..485149e 100644
--- a/src/lib/krb5/os/changepw.c
+++ b/src/lib/krb5/os/changepw.c
@@ -57,8 +57,8 @@ krb5_change_password(context, creds, newpw, result_code,
     krb5_address local_kaddr, remote_kaddr;
     const char *realm_kdc_names[4];
     int default_port;
-    char **hostlist, *host, *port, *cp, *code_string;
-    krb5_error_code code;
+    char **hostlist, *host, *tmphost, *port, *cp, *code_string;
+    krb5_error_code code = 0;
     int i, j, out, count, addrlen;
     struct sockaddr *addr_p, local_addr, remote_addr, tmp_addr;
     struct sockaddr_in *sin_p;
@@ -68,17 +68,30 @@ krb5_change_password(context, creds, newpw, result_code,
     u_short udpport = htons(KRB5_DEFAULT_PORT);
 #endif
     int cc, local_result_code, tmp_len;
-    SOCKET s1, s2;
+    SOCKET s1 = INVALID_SOCKET, s2 = INVALID_SOCKET;
 
+    /* Initialize values so that cleanup call can safely check for NULL */
     auth_context = NULL;
-
+    addr_p = NULL;
+    host = NULL;
+    hostlist = NULL;
+    memset(&chpw_req, 0, sizeof(krb5_data));
+    memset(&chpw_rep, 0, sizeof(krb5_data));
+    memset(&ap_req, 0, sizeof(krb5_data));
+    
+    /* initialize auth_context so that we know we have to free it */
+    if ((code = krb5_auth_con_init(context, &auth_context)))
+	  goto cleanup;
+    
     if (code = krb5_mk_req_extended(context, &auth_context, AP_OPTS_USE_SUBKEY,
 				    NULL, creds, &ap_req))
-	return(code);
+	  goto cleanup;
 
-    if ((host = malloc(krb5_princ_realm(context, creds->client)->length + 1))
-	== NULL) 
-	return ENOMEM;
+    if ((host = malloc(krb5_princ_realm(context, creds->client)->length + 1)) == NULL) 
+	  {
+	    code = ENOMEM;
+	    goto cleanup;
+	  }
 
     strncpy(host, krb5_princ_realm(context, creds->client)->data,
 	    krb5_princ_realm(context, creds->client)->length);
@@ -94,23 +107,27 @@ krb5_change_password(context, creds, newpw, result_code,
 
     code = profile_get_values(context->profile, realm_kdc_names, &hostlist);
 
-    if (code == PROF_NO_RELATION) {
-	realm_kdc_names[2] = "admin_server";
-
-	default_port = 1;
-
-	code = profile_get_values(context->profile, realm_kdc_names,
-				  &hostlist);
-    }
-
-    krb5_xfree(host);
+    if (code == PROF_NO_RELATION) 
+      {
+        realm_kdc_names[2] = "admin_server";
+        default_port = 1;
+        code = profile_get_values(context->profile, realm_kdc_names, &hostlist);
+      }
 
     if (code == PROF_NO_SECTION)
-	return KRB5_REALM_UNKNOWN;
-    else if (code == PROF_NO_RELATION)
-	return KRB5_CONFIG_BADFORMAT;
-    else if (code)
-	return code;
+      {
+        code = KRB5_REALM_UNKNOWN;
+        goto cleanup;
+      }
+    else 
+      if (code == PROF_NO_RELATION)
+        {
+          code = KRB5_CONFIG_BADFORMAT;
+          goto cleanup;
+        }
+      else 
+        if (code)
+          goto cleanup;
 
 #ifdef HAVE_NETINET_IN_H
     /* XXX should look for "kpasswd" in /etc/services */
@@ -122,28 +139,34 @@ krb5_change_password(context, creds, newpw, result_code,
 	    count++;
     
     if (count == 0)
-	/* XXX */
-	return(KADM_NO_HOST);
+      {
+        /* XXX */
+        code = KADM_NO_HOST;
+        goto cleanup;
+      }
     
     addr_p = (struct sockaddr *) malloc(sizeof(struct sockaddr) * count);
     if (addr_p == NULL)
-        return ENOMEM;
+      {
+        code = ENOMEM;
+        goto cleanup;
+      }
 
-    host = hostlist[0];
+    tmphost = hostlist[0];
     out = 0;
 
     /*
      * Strip off excess whitespace
      */
-    cp = strchr(host, ' ');
+    cp = strchr(tmphost, ' ');
     if (cp)
-	*cp = 0;
-    cp = strchr(host, '\t');
+      *cp = 0;
+    cp = strchr(tmphost, '\t');
     if (cp)
-	*cp = 0;
-    port = strchr(host, ':');
+      *cp = 0;
+    port = strchr(tmphost, ':');
     if (port) {
-	*port = 0;
+      *port = 0;
 	port++;
 	/* if the admin_server line was used, ignore the specified
            port */
@@ -152,40 +175,46 @@ krb5_change_password(context, creds, newpw, result_code,
     }
     hp = gethostbyname(hostlist[0]);
 
-    if (hp != 0) {
-	switch (hp->h_addrtype) {
+    if (hp != 0) 
+      {
+        switch (hp->h_addrtype) 
+          {
 #ifdef HAVE_NETINET_IN_H
-	case AF_INET:
-	    for (j=0; hp->h_addr_list[j]; j++) {
-		sin_p = (struct sockaddr_in *) &addr_p[out++];
-		memset ((char *)sin_p, 0, sizeof(struct sockaddr));
-		sin_p->sin_family = hp->h_addrtype;
-		sin_p->sin_port = port ? htons(atoi(port)) : udpport;
-		memcpy((char *)&sin_p->sin_addr,
-		       (char *)hp->h_addr_list[j],
-		       sizeof(struct in_addr));
-		if (out+1 >= count) {
-		    count += 5;
-		    addr_p = (struct sockaddr *)
-			realloc ((char *)addr_p,
-				 sizeof(struct sockaddr) * count);
-		    if (addr_p == NULL)
-			return ENOMEM;
-		}
-	    }
-	    break;
+          case AF_INET:
+            for (j=0; hp->h_addr_list[j]; j++) 
+              {
+                sin_p = (struct sockaddr_in *) &addr_p[out++];
+                memset ((char *)sin_p, 0, sizeof(struct sockaddr));
+                sin_p->sin_family = hp->h_addrtype;
+                sin_p->sin_port = port ? htons(atoi(port)) : udpport;
+                memcpy((char *)&sin_p->sin_addr,
+                       (char *)hp->h_addr_list[j],
+                       sizeof(struct in_addr));
+                if (out+1 >= count) 
+                  {
+                    count += 5;
+                    addr_p = (struct sockaddr *)
+                    realloc ((char *)addr_p, sizeof(struct sockaddr) * count);
+                    if (addr_p == NULL)
+                      {
+                        code = ENOMEM;
+                        goto cleanup;
+                      }
+                  }
+                }
+              break;
 #endif
-	default:
-	    break;
-	}
-    }
-
-    profile_free_list(hostlist);
-
-    if (out == 0) {     /* Couldn't resolve any KDC names */
-        free (addr_p);
-        return(KADM_NO_HOST);
-    }
+          default:
+            break;
+        }
+      }
+
+    if (out == 0) 
+      {     
+        /* Couldn't resolve any KDC names */
+        code = KADM_NO_HOST;
+        goto cleanup;
+      }
 
     /* this is really obscure.  s1 is used for all communications.  it
        is left unconnected in case the server is multihomed and routes
@@ -203,187 +232,194 @@ krb5_change_password(context, creds, newpw, result_code,
        hostname resolution to get the local ip addr) will work and
        interoperate if the client is single-homed. */
 
-    if ((s1 = socket(AF_INET, SOCK_DGRAM, 0)) == INVALID_SOCKET) {
-	free(addr_p);
-	return(SOCKET_ERRNO);
-    }
-
-    if ((s2 = socket(AF_INET, SOCK_DGRAM, 0)) == INVALID_SOCKET) {
-	free(addr_p);
-	return(SOCKET_ERRNO);
-    }
-
-    for (i=0; i<out; i++) {
-	if (connect(s2, &addr_p[i], sizeof(addr_p[i])) == SOCKET_ERROR) {
-	    if ((cc < 0) && ((SOCKET_ERRNO == ECONNREFUSED) ||
-			     (SOCKET_ERRNO == EHOSTUNREACH)))
-		continue; /* try the next addr */
-	    free(addr_p);
-	    closesocket(s1);
-	    closesocket(s2);
-	    return(SOCKET_ERRNO);
-	}
-
-	addrlen = sizeof(local_addr);
-
-	if (getsockname(s2, &local_addr, &addrlen) < 0) {
-	    if ((SOCKET_ERRNO == ECONNREFUSED) ||
-		(SOCKET_ERRNO == EHOSTUNREACH))
-		continue; /* try the next addr */
-	    free(addr_p);
-	    closesocket(s1);
-	    closesocket(s2);
-	    return(SOCKET_ERRNO);
-	}
-
-	/* some brain-dead OS's don't return useful information from
-	 * the getsockname call.  Namely, windows and solaris.  */
-
-	if (((struct sockaddr_in *)&local_addr)->sin_addr.s_addr != 0) {
-	    local_kaddr.addrtype = ADDRTYPE_INET;
-	    local_kaddr.length =
-	      sizeof(((struct sockaddr_in *) &local_addr)->sin_addr);
-	    local_kaddr.contents = 
-	      (krb5_octet *) &(((struct sockaddr_in *) &local_addr)->sin_addr);
-	} else {
-	    krb5_address **addrs;
-
-	    krb5_os_localaddr(context, &addrs);
-	    local_kaddr.magic = addrs[0]->magic;
-	    local_kaddr.addrtype = addrs[0]->addrtype;
-	    local_kaddr.length = addrs[0]->length;
-	    local_kaddr.contents = malloc(addrs[0]->length);
-	    memcpy(local_kaddr.contents, addrs[0]->contents, addrs[0]->length);
-
-	    krb5_free_addresses(context, addrs);
-	}
-
-	addrlen = sizeof(remote_addr);
-	if (getpeername(s2, &remote_addr, &addrlen) < 0) {
-	    if ((SOCKET_ERRNO == ECONNREFUSED) ||
-		(SOCKET_ERRNO == EHOSTUNREACH))
-		continue; /* try the next addr */
-	    free(addr_p);
-	    closesocket(s1);
-	    closesocket(s2);
-	    return(SOCKET_ERRNO);
-	}
-
-	remote_kaddr.addrtype = ADDRTYPE_INET;
-	remote_kaddr.length =
-	    sizeof(((struct sockaddr_in *) &remote_addr)->sin_addr);
-	remote_kaddr.contents = 
-	    (krb5_octet *) &(((struct sockaddr_in *) &remote_addr)->sin_addr);
-
-	/* mk_priv requires that the local address be set.
-	  getsockname is used for this.  rd_priv requires that the
-	  remote address be set.  recvfrom is used for this.  If
-	  rd_priv is given a local address, and the message has the
-	  recipient addr in it, this will be checked.  However, there
-	  is simply no way to know ahead of time what address the
-	  message will be delivered *to*.  Therefore, it is important
-	  that either no recipient address is in the messages when
-	  mk_priv is called, or that no local address is passed to
-	  rd_priv.  Both is a better idea, and I have done that.  In
-	  summary, when mk_priv is called, *only* a local address is
-	  specified.  when rd_priv is called, *only* a remote address
-	  is specified.  Are we having fun yet?  */
-
-	if (code = krb5_auth_con_setaddrs(context, auth_context, &local_kaddr,
-					  NULL)) {
-	    free(addr_p);
-	    closesocket(s1);
-	    closesocket(s2);
-	    return(code);
-	}
-
-	if (code = krb5_mk_chpw_req(context, auth_context, &ap_req,
-				    newpw, &chpw_req)) {
-	    free(addr_p);
-	    closesocket(s1);
-	    closesocket(s2);
-	    return(code);
-	}
-
-	if ((cc = sendto(s1, chpw_req.data, chpw_req.length, 0,
-			 (struct sockaddr *) &addr_p[i],
-			 sizeof(addr_p[i]))) !=
-	    chpw_req.length) {
-	    if ((cc < 0) && ((SOCKET_ERRNO == ECONNREFUSED) ||
-			     (SOCKET_ERRNO == EHOSTUNREACH)))
-		continue; /* try the next addr */
-	    free(addr_p);
-	    closesocket(s1);
-	    closesocket(s2);
-	    return((cc < 0)?SOCKET_ERRNO:ECONNABORTED);
-	}
-
-	krb5_xfree(chpw_req.data);
-
-	chpw_rep.length = 1500;
-	chpw_rep.data = (char *) malloc(chpw_rep.length);
-
-	/* XXX need a timeout/retry loop here */
-
-	/* "recv" would be good enough here... except that Windows/NT
-	   commits the atrocity of returning -1 to indicate failure,
-	   but leaving errno set to 0.
-	   
-	   "recvfrom(...,NULL,NULL)" would seem to be a good enough
-	   alternative, and it works on NT, but it doesn't work on
-	   SunOS 4.1.4 or Irix 5.3.  Thus we must actually accept the
-	   value and discard it. */
-	tmp_len = sizeof(tmp_addr);
-	if ((cc = recvfrom(s1, chpw_rep.data, chpw_rep.length, 0, &tmp_addr, &tmp_len)) < 0) {
-	    free(addr_p);
-	    closesocket(s1);
-	    closesocket(s2);
-	    return(SOCKET_ERRNO);
-	}
-
-	closesocket(s1);
-	closesocket(s2);
-
-	chpw_rep.length = cc;
-
-	if (code = krb5_auth_con_setaddrs(context, auth_context, NULL,
-					  &remote_kaddr)) {
-	    free(addr_p);
-	    closesocket(s1);
-	    closesocket(s2);
-	    return(code);
-	}
-
-	code = krb5_rd_chpw_rep(context, auth_context, &chpw_rep,
-				&local_result_code, result_string);
-
-	free(chpw_rep.data);
-	free(addr_p);
-
-	if (code)
-	    return(code);
-
-	if (result_code)
-	    *result_code = local_result_code;
-
-	if (result_code_string) {
-	    if (code = krb5_chpw_result_code_string(context, local_result_code,
-						    &code_string))
-		return(code);
-
-	    result_code_string->length = strlen(code_string);
-	    if ((result_code_string->data =
-		 (char *) malloc(result_code_string->length)) == NULL)
-		return(ENOMEM);
-	    strncpy(result_code_string->data, code_string,
-		    result_code_string->length);
-	}
-
-	return(0);
-    }
-
-    free(addr_p);
-    closesocket(s1);
-    closesocket(s2);
-    return(SOCKET_ERRNO);
+    if ((s1 = socket(AF_INET, SOCK_DGRAM, 0)) == INVALID_SOCKET) 
+      {
+	    code = SOCKET_ERRNO;
+	    goto cleanup;
+      }
+
+    if ((s2 = socket(AF_INET, SOCK_DGRAM, 0)) == INVALID_SOCKET)
+      {
+	    code = SOCKET_ERRNO;
+	    goto cleanup;
+      }
+
+    for (i=0; i<out; i++) 
+      {
+		if (connect(s2, &addr_p[i], sizeof(addr_p[i])) == SOCKET_ERROR) 
+		  {
+		    if ((SOCKET_ERRNO == ECONNREFUSED) || (SOCKET_ERRNO == EHOSTUNREACH))
+			  continue; /* try the next addr */
+		    
+		    code = SOCKET_ERRNO;
+		    goto cleanup;
+		  }
+      
+        addrlen = sizeof(local_addr);
+
+		if (getsockname(s2, &local_addr, &addrlen) < 0) 
+		  {
+		    if ((SOCKET_ERRNO == ECONNREFUSED) || (SOCKET_ERRNO == EHOSTUNREACH))
+			  continue; /* try the next addr */
+		    
+		    code = SOCKET_ERRNO;
+			goto cleanup;
+		  }
+
+		/* some brain-dead OS's don't return useful information from
+		 * the getsockname call.  Namely, windows and solaris.  */
+
+		if (((struct sockaddr_in *)&local_addr)->sin_addr.s_addr != 0) 
+		  {
+		    local_kaddr.addrtype = ADDRTYPE_INET;
+		    local_kaddr.length = sizeof(((struct sockaddr_in *) &local_addr)->sin_addr);
+		    local_kaddr.contents = (krb5_octet *) &(((struct sockaddr_in *) &local_addr)->sin_addr);
+		  } 
+		else 
+		  {
+		    krb5_address **addrs;
+
+		    krb5_os_localaddr(context, &addrs);
+		    
+		    local_kaddr.magic = addrs[0]->magic;
+		    local_kaddr.addrtype = addrs[0]->addrtype;
+		    local_kaddr.length = addrs[0]->length;
+		    local_kaddr.contents = malloc(addrs[0]->length);
+		    memcpy(local_kaddr.contents, addrs[0]->contents, addrs[0]->length);
+
+		    krb5_free_addresses(context, addrs);
+		  }
+
+		addrlen = sizeof(remote_addr);
+		if (getpeername(s2, &remote_addr, &addrlen) < 0) 
+		  {
+		    if ((SOCKET_ERRNO == ECONNREFUSED) || (SOCKET_ERRNO == EHOSTUNREACH))
+			  continue; /* try the next addr */
+		    
+		    code = SOCKET_ERRNO;
+			goto cleanup;
+		  }
+
+		remote_kaddr.addrtype = ADDRTYPE_INET;
+		remote_kaddr.length = sizeof(((struct sockaddr_in *) &remote_addr)->sin_addr);
+		remote_kaddr.contents = (krb5_octet *) &(((struct sockaddr_in *) &remote_addr)->sin_addr);
+
+		/* mk_priv requires that the local address be set.
+		  getsockname is used for this.  rd_priv requires that the
+		  remote address be set.  recvfrom is used for this.  If
+		  rd_priv is given a local address, and the message has the
+		  recipient addr in it, this will be checked.  However, there
+		  is simply no way to know ahead of time what address the
+		  message will be delivered *to*.  Therefore, it is important
+		  that either no recipient address is in the messages when
+		  mk_priv is called, or that no local address is passed to
+		  rd_priv.  Both is a better idea, and I have done that.  In
+		  summary, when mk_priv is called, *only* a local address is
+		  specified.  when rd_priv is called, *only* a remote address
+		  is specified.  Are we having fun yet?  */
+
+		if (code = krb5_auth_con_setaddrs(context, auth_context, &local_kaddr, NULL)) 
+		  {
+		    code = SOCKET_ERRNO;
+			goto cleanup;
+		  }
+
+		if (code = krb5_mk_chpw_req(context, auth_context, &ap_req, newpw, &chpw_req)) 
+		  {
+		    code = SOCKET_ERRNO;
+			goto cleanup;
+		  }
+
+		if ((cc = sendto(s1, chpw_req.data, chpw_req.length, 0, 
+		                 (struct sockaddr *) &addr_p[i], 
+		                 sizeof(addr_p[i]))) != chpw_req.length) 
+		  {
+		    if ((cc < 0) && ((SOCKET_ERRNO == ECONNREFUSED) ||
+				     (SOCKET_ERRNO == EHOSTUNREACH)))
+			  continue; /* try the next addr */
+		    
+		    code = (cc < 0) ? SOCKET_ERRNO : ECONNABORTED;
+			goto cleanup;
+		  }
+
+		chpw_rep.length = 1500;
+		chpw_rep.data = (char *) malloc(chpw_rep.length);
+
+		/* XXX need a timeout/retry loop here */
+
+		/* "recv" would be good enough here... except that Windows/NT
+		   commits the atrocity of returning -1 to indicate failure,
+		   but leaving errno set to 0.
+		   
+		   "recvfrom(...,NULL,NULL)" would seem to be a good enough
+		   alternative, and it works on NT, but it doesn't work on
+		   SunOS 4.1.4 or Irix 5.3.  Thus we must actually accept the
+		   value and discard it. */
+		tmp_len = sizeof(tmp_addr);
+		if ((cc = recvfrom(s1, chpw_rep.data, chpw_rep.length, 0, &tmp_addr, &tmp_len)) < 0) 
+		  {
+		    code = SOCKET_ERRNO;
+		    goto cleanup;
+		  }
+
+		closesocket(s1);
+		s1 = INVALID_SOCKET;
+		closesocket(s2);
+		s2 = INVALID_SOCKET;
+
+		chpw_rep.length = cc;
+
+		if (code = krb5_auth_con_setaddrs(context, auth_context, NULL, &remote_kaddr)) 
+		  goto cleanup;
+
+		if(code = krb5_rd_chpw_rep(context, auth_context, &chpw_rep,
+					&local_result_code, result_string))
+		  goto cleanup;
+
+		if (result_code)
+		  *result_code = local_result_code;
+
+		if (result_code_string) 
+		  {
+		    if (code = krb5_chpw_result_code_string(context, local_result_code,
+							    &code_string))
+			  goto cleanup;
+
+		    result_code_string->length = strlen(code_string);
+		    if ((result_code_string->data =
+			    (char *) malloc(result_code_string->length)) == NULL)
+			  return(ENOMEM);
+		    strncpy(result_code_string->data, code_string, result_code_string->length);
+		  }
+
+		code = 0;
+		goto cleanup;
+      }
+
+    code = SOCKET_ERRNO;
+    
+cleanup:
+    if(auth_context != NULL)
+      krb5_auth_con_free(context, auth_context);
+    
+    if(host != NULL)
+      krb5_xfree(host);
+    
+    if(addr_p != NULL)
+      krb5_xfree(addr_p);
+    
+    if(hostlist != NULL)
+      profile_free_list(hostlist);
+      
+    if(s1 != INVALID_SOCKET)
+      closesocket(s1);
+    
+    if(s2 != INVALID_SOCKET)
+      closesocket(s2);
+      
+    krb5_free_data_contents(context, &chpw_req);
+    krb5_free_data_contents(context, &chpw_rep);
+	krb5_free_data_contents(context, &ap_req);
+    
+    return(code);
 }