Back out patch pending review.

---------------------------------------------------------------------------

>   I've now tested this patch at home w/ 8.2HEAD and it seems to fix the
>   bug.  I plan on testing it under 8.1.2 at work tommorow with
>   mod_auth_krb5, etc, and expect it'll work there.  Assuming all goes
>   well and unless someone objects I'll forward the patch to -patches.
>   It'd be great to have this fixed as it'll allow us to use Kerberos to
>   authenticate to phppgadmin and other web-based tools which use
>   Postgres.

  While playing with this patch under 8.1.2 at home I discovered a
  mistake in how I manually applied one of the hunks to fe-auth.c.
  Basically, the base code had changed and so the patch needed to be
  modified slightly.  This is because the code no longer either has a
  freeable pointer under 'name' or has 'name' as NULL.

  The attached patch correctly frees the string from pg_krb5_authname
  (where it had been strdup'd) if and only if pg_krb5_authname returned
  a string (as opposed to falling through and having name be set using
  name = pw->name;).  Also added a comment to this effect.
  Please review.

Stephen Frost (sfrost@snowman.net) wrote:

1 files changed, 27 insertions, 73 deletions

diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 4ad50a7a48d..0996e0de329 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -10,7 +10,7 @@
  * exceed INITIAL_EXPBUFFER_SIZE (currently 256 bytes).
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/interfaces/libpq/fe-auth.c,v 1.111 2006/02/12 20:04:42 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/interfaces/libpq/fe-auth.c,v 1.112 2006/02/12 20:08:29 momjian Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -101,33 +101,22 @@ pg_an_to_ln(char *aname)
  * Various krb5 state which is not connection specific, and a flag to
  * indicate whether we have initialised it yet.
  */
-/* 
 static int	pg_krb5_initialised;
 static krb5_context pg_krb5_context;
 static krb5_ccache pg_krb5_ccache;
 static krb5_principal pg_krb5_client;
 static char *pg_krb5_name;
-*/
-
-struct krb5_info
-{
-	int		pg_krb5_initialised;
-	krb5_context	pg_krb5_context;
-	krb5_ccache	pg_krb5_ccache;
-	krb5_principal	pg_krb5_client;
-	char		*pg_krb5_name;
-};
 
 
 static int
-pg_krb5_init(char *PQerrormsg, struct krb5_info *info)
+pg_krb5_init(char *PQerrormsg)
 {
 	krb5_error_code retval;
 
-	if (info->pg_krb5_initialised)
+	if (pg_krb5_initialised)
 		return STATUS_OK;
 
-	retval = krb5_init_context(&(info->pg_krb5_context));
+	retval = krb5_init_context(&pg_krb5_context);
 	if (retval)
 	{
 		snprintf(PQerrormsg, PQERRORMSG_LENGTH,
@@ -136,56 +125,46 @@ pg_krb5_init(char *PQerrormsg, struct krb5_info *info)
 		return STATUS_ERROR;
 	}
 
-	retval = krb5_cc_default(info->pg_krb5_context, &(info->pg_krb5_ccache));
+	retval = krb5_cc_default(pg_krb5_context, &pg_krb5_ccache);
 	if (retval)
 	{
 		snprintf(PQerrormsg, PQERRORMSG_LENGTH,
 				 "pg_krb5_init: krb5_cc_default: %s\n",
 				 error_message(retval));
-		krb5_free_context(info->pg_krb5_context);
+		krb5_free_context(pg_krb5_context);
 		return STATUS_ERROR;
 	}
 
-	retval = krb5_cc_get_principal(info->pg_krb5_context, info->pg_krb5_ccache,
-								   &(info->pg_krb5_client));
+	retval = krb5_cc_get_principal(pg_krb5_context, pg_krb5_ccache,
+								   &pg_krb5_client);
 	if (retval)
 	{
 		snprintf(PQerrormsg, PQERRORMSG_LENGTH,
 				 "pg_krb5_init: krb5_cc_get_principal: %s\n",
 				 error_message(retval));
-		krb5_cc_close(info->pg_krb5_context, info->pg_krb5_ccache);
-		krb5_free_context(info->pg_krb5_context);
+		krb5_cc_close(pg_krb5_context, pg_krb5_ccache);
+		krb5_free_context(pg_krb5_context);
 		return STATUS_ERROR;
 	}
 
-	retval = krb5_unparse_name(info->pg_krb5_context, info->pg_krb5_client, &(info->pg_krb5_name));
+	retval = krb5_unparse_name(pg_krb5_context, pg_krb5_client, &pg_krb5_name);
 	if (retval)
 	{
 		snprintf(PQerrormsg, PQERRORMSG_LENGTH,
 				 "pg_krb5_init: krb5_unparse_name: %s\n",
 				 error_message(retval));
-		krb5_free_principal(info->pg_krb5_context, info->pg_krb5_client);
-		krb5_cc_close(info->pg_krb5_context, info->pg_krb5_ccache);
-		krb5_free_context(info->pg_krb5_context);
+		krb5_free_principal(pg_krb5_context, pg_krb5_client);
+		krb5_cc_close(pg_krb5_context, pg_krb5_ccache);
+		krb5_free_context(pg_krb5_context);
 		return STATUS_ERROR;
 	}
 
-	info->pg_krb5_name = pg_an_to_ln(info->pg_krb5_name);
+	pg_krb5_name = pg_an_to_ln(pg_krb5_name);
 
-	info->pg_krb5_initialised = 1;
+	pg_krb5_initialised = 1;
 	return STATUS_OK;
 }
 
-static void 
-pg_krb5_destroy(struct krb5_info *info)
-{
-	krb5_free_principal(info->pg_krb5_context, info->pg_krb5_client);
-	krb5_cc_close(info->pg_krb5_context, info->pg_krb5_ccache);
-	krb5_free_context(info->pg_krb5_context);
-	free(info->pg_krb5_name);
-}
-
-
 
 /*
  * pg_krb5_authname -- returns a pointer to static space containing whatever
@@ -194,16 +173,10 @@ pg_krb5_destroy(struct krb5_info *info)
 static const char *
 pg_krb5_authname(char *PQerrormsg)
 {
-	char *tmp_name;
-	struct krb5_info info;
-	info.pg_krb5_initialised = 0;
-
-	if (pg_krb5_init(PQerrormsg, &info) != STATUS_OK)
+	if (pg_krb5_init(PQerrormsg) != STATUS_OK)
 		return NULL;
-	tmp_name = strdup(info.pg_krb5_name);
-	pg_krb5_destroy(&info);
 
-	return tmp_name;
+	return pg_krb5_name;
 }
 
 
@@ -219,8 +192,6 @@ pg_krb5_sendauth(char *PQerrormsg, int sock, const char *hostname, const char *s
 	krb5_principal server;
 	krb5_auth_context auth_context = NULL;
 	krb5_error *err_ret = NULL;
-	struct krb5_info info;
-	info.pg_krb5_initialised = 0;
 
 	if (!hostname)
 	{
@@ -229,18 +200,17 @@ pg_krb5_sendauth(char *PQerrormsg, int sock, const char *hostname, const char *s
 		return STATUS_ERROR;
 	}
 
-	ret = pg_krb5_init(PQerrormsg, &info);
+	ret = pg_krb5_init(PQerrormsg);
 	if (ret != STATUS_OK)
 		return ret;
 
-	retval = krb5_sname_to_principal(info.pg_krb5_context, hostname, servicename,
+	retval = krb5_sname_to_principal(pg_krb5_context, hostname, servicename,
 									 KRB5_NT_SRV_HST, &server);
 	if (retval)
 	{
 		snprintf(PQerrormsg, PQERRORMSG_LENGTH,
 				 "pg_krb5_sendauth: krb5_sname_to_principal: %s\n",
 				 error_message(retval));
-		pg_krb5_destroy(&info);
 		return STATUS_ERROR;
 	}
 
@@ -255,17 +225,16 @@ pg_krb5_sendauth(char *PQerrormsg, int sock, const char *hostname, const char *s
 
 		snprintf(PQerrormsg, PQERRORMSG_LENGTH,
 				 libpq_gettext("could not set socket to blocking mode: %s\n"), pqStrerror(errno, sebuf, sizeof(sebuf)));
-		krb5_free_principal(info.pg_krb5_context, server);
-		pg_krb5_destroy(&info);
+		krb5_free_principal(pg_krb5_context, server);
 		return STATUS_ERROR;
 	}
 
-	retval = krb5_sendauth(info.pg_krb5_context, &auth_context,
+	retval = krb5_sendauth(pg_krb5_context, &auth_context,
 						   (krb5_pointer) & sock, (char *) servicename,
-						   info.pg_krb5_client, server,
+						   pg_krb5_client, server,
 						   AP_OPTS_MUTUAL_REQUIRED,
 						   NULL, 0,		/* no creds, use ccache instead */
-						   info.pg_krb5_ccache, &err_ret, NULL, NULL);
+						   pg_krb5_ccache, &err_ret, NULL, NULL);
 	if (retval)
 	{
 		if (retval == KRB5_SENDAUTH_REJECTED && err_ret)
@@ -290,12 +259,12 @@ pg_krb5_sendauth(char *PQerrormsg, int sock, const char *hostname, const char *s
 		}
 
 		if (err_ret)
-			krb5_free_error(info.pg_krb5_context, err_ret);
+			krb5_free_error(pg_krb5_context, err_ret);
 
 		ret = STATUS_ERROR;
 	}
 
-	krb5_free_principal(info.pg_krb5_context, server);
+	krb5_free_principal(pg_krb5_context, server);
 
 	if (!pg_set_noblock(sock))
 	{
@@ -306,7 +275,6 @@ pg_krb5_sendauth(char *PQerrormsg, int sock, const char *hostname, const char *s
 				 pqStrerror(errno, sebuf, sizeof(sebuf)));
 		ret = STATUS_ERROR;
 	}
-	pg_krb5_destroy(&info);
 
 	return ret;
 }
@@ -519,9 +487,6 @@ pg_fe_sendauth(AuthRequest areq, PGconn *conn, const char *hostname,
 char *
 pg_fe_getauthname(char *PQerrormsg)
 {
-#ifdef KRB5
-	const char *krb5_name = NULL;
-#endif
 	const char *name = NULL;
 	char	   *authn;
 
@@ -546,12 +511,7 @@ pg_fe_getauthname(char *PQerrormsg)
 	pglock_thread();
 
 #ifdef KRB5
-	/* pg_krb5_authname gives us a strdup'd value that we need
-	 * to free later, however, we don't want to free 'name' directly
-	 * in case it's *not* a Kerberos login and we fall through to
-	 * name = pw->pw_name; */
-	krb5_name = pg_krb5_authname(PQerrormsg);
-	name = krb5_name;
+	name = pg_krb5_authname(PQerrormsg);
 #endif
 
 	if (!name)
@@ -567,12 +527,6 @@ pg_fe_getauthname(char *PQerrormsg)
 
 	authn = name ? strdup(name) : NULL;
 
-#ifdef KRB5
-	/* Free the strdup'd string from pg_krb5_authname, if we got one */
-	if (krb5_name)
-		free(krb5_name);
-#endif
-
 	pgunlock_thread();
 
 	return authn;