[04/10] gssd: gssd_k5_err_msg() returns a strdup'd msg. Use free() to release.
diff mbox series

Message ID 20200701182803.14947-5-nazard@nazar.ca
State New
Headers show
Series
  • Misc fixes & cleanups for nfs-utils
Related show

Commit Message

Doug Nazar July 1, 2020, 6:27 p.m. UTC
Signed-off-by: Doug Nazar <nazard@nazar.ca>
---
 utils/gssd/krb5_util.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Steve Dickson July 8, 2020, 2:50 p.m. UTC | #1
Hello,

On 7/1/20 2:27 PM, Doug Nazar wrote:
> Signed-off-by: Doug Nazar <nazard@nazar.ca>
> ---
>  utils/gssd/krb5_util.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
> index c49c6672..b1e48241 100644
> --- a/utils/gssd/krb5_util.c
> +++ b/utils/gssd/krb5_util.c
> @@ -484,7 +484,7 @@ gssd_get_single_krb5_cred(krb5_context context,
>  	if (ccache)
>  		krb5_cc_close(context, ccache);
>  	krb5_free_cred_contents(context, &my_creds);
> -	krb5_free_string(context, k5err);
> +	free(k5err);
>  	return (code);
>  }
>  
> @@ -723,7 +723,7 @@ gssd_search_krb5_keytab(krb5_context context, krb5_keytab kt,
>  				 "we failed to unparse principal name: %s\n",
>  				 k5err);
>  			k5_free_kt_entry(context, kte);
> -			krb5_free_string(context, k5err);
> +			free(k5err);
>  			k5err = NULL;
>  			continue;
>  		}
> @@ -770,7 +770,7 @@ gssd_search_krb5_keytab(krb5_context context, krb5_keytab kt,
>  	if (retval < 0)
>  		retval = 0;
>    out:
> -	krb5_free_string(context, k5err);
> +	free(k5err);
>  	return retval;
>  }
>  
> @@ -927,7 +927,7 @@ find_keytab_entry(krb5_context context, krb5_keytab kt,
>  				k5err = gssd_k5_err_msg(context, code);
>  				printerr(1, "%s while building principal for '%s'\n",
>  					 k5err, spn);
> -				krb5_free_string(context, k5err);
> +				free(k5err);
>  				k5err = NULL;
>  				continue;
>  			}
> @@ -937,7 +937,7 @@ find_keytab_entry(krb5_context context, krb5_keytab kt,
>  				k5err = gssd_k5_err_msg(context, code);
>  				printerr(3, "%s while getting keytab entry for '%s'\n",
>  					 k5err, spn);
> -				krb5_free_string(context, k5err);
> +				free(k5err);
>  				k5err = NULL;
>  				/*
>  				 * We tried the active directory machine account
> @@ -986,7 +986,7 @@ out:
>  		k5_free_default_realm(context, default_realm);
>  	if (realmnames)
>  		krb5_free_host_realm(context, realmnames);
> -	krb5_free_string(context, k5err);
> +	free(k5err);
>  	return retval;
>  }
>  
> @@ -1355,7 +1355,7 @@ out_free_kt:
>  out_free_context:
>  	krb5_free_context(context);
>  out:
> -	krb5_free_string(context, k5err);
> +	free(k5err);
>  	return retval;
>  }
>  
> 
I'm curious about these changes... since all krb5_free_string()
does is call free()... where is the "strdup'd msg" coming from?

steved.
Doug Nazar July 12, 2020, 8:27 p.m. UTC | #2
On 2020-07-08 10:50, Steve Dickson wrote:
> I'm curious about these changes... since all krb5_free_string()
> does is call free()... where is the "strdup'd msg" coming from?

gssd_k5_err_msg() always returns a local strdup() of the error message. 
We shouldn't be using a Kerberos library method to free them. There's no 
guarantee that the library won't change, or even that the library was 
compiled with the same malloc library.

Doug
Steve Dickson July 13, 2020, 6:47 p.m. UTC | #3
On 7/12/20 4:27 PM, Doug Nazar wrote:
> On 2020-07-08 10:50, Steve Dickson wrote:
>> I'm curious about these changes... since all krb5_free_string()
>> does is call free()... where is the "strdup'd msg" coming from?
> 
> gssd_k5_err_msg() always returns a local strdup() of the error message. 
True... 

> We shouldn't be using a Kerberos library method to free them. There's no guarantee that the library won't change, 
I guess I'm not too worry about this... I would the change was for the better.
and lets face the krb5 has not changed in a 100 years ;-) 

> or even that the library was compiled with the same malloc library.
There are different malloc libraries other than glibc? 

steved.
Doug Nazar July 13, 2020, 10:22 p.m. UTC | #4
On 2020-07-13 14:47, Steve Dickson wrote:
> or even that the library was compiled with the same malloc library.
> There are different malloc libraries other than glibc?

Not including the various libc implementations (dietlibc, klibc, uclibc) 
there are several specialized malloc libraries (dmalloc & jemalloc are 
the most common) that you can compile or link against. Usually since 
they replace malloc and friends everything works, but you can get into 
issues depending on order of linking and how you compile.

I recently put gssd though a few rounds of mallocfail, which allows you 
to progressive test every allocation point (during normal running) to 
ensure it's handled (instead of crashing). I've an updated patchset that 
fixes the event2 conversion to check for successful allocation and a 
couple other spots that crashed if the allocation failed.

I'm taking it a bit slower this time... kinda rushed the last patchset 
out the door since I was needed on another project.  ;-)

Doug

Patch
diff mbox series

diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index c49c6672..b1e48241 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -484,7 +484,7 @@  gssd_get_single_krb5_cred(krb5_context context,
 	if (ccache)
 		krb5_cc_close(context, ccache);
 	krb5_free_cred_contents(context, &my_creds);
-	krb5_free_string(context, k5err);
+	free(k5err);
 	return (code);
 }
 
@@ -723,7 +723,7 @@  gssd_search_krb5_keytab(krb5_context context, krb5_keytab kt,
 				 "we failed to unparse principal name: %s\n",
 				 k5err);
 			k5_free_kt_entry(context, kte);
-			krb5_free_string(context, k5err);
+			free(k5err);
 			k5err = NULL;
 			continue;
 		}
@@ -770,7 +770,7 @@  gssd_search_krb5_keytab(krb5_context context, krb5_keytab kt,
 	if (retval < 0)
 		retval = 0;
   out:
-	krb5_free_string(context, k5err);
+	free(k5err);
 	return retval;
 }
 
@@ -927,7 +927,7 @@  find_keytab_entry(krb5_context context, krb5_keytab kt,
 				k5err = gssd_k5_err_msg(context, code);
 				printerr(1, "%s while building principal for '%s'\n",
 					 k5err, spn);
-				krb5_free_string(context, k5err);
+				free(k5err);
 				k5err = NULL;
 				continue;
 			}
@@ -937,7 +937,7 @@  find_keytab_entry(krb5_context context, krb5_keytab kt,
 				k5err = gssd_k5_err_msg(context, code);
 				printerr(3, "%s while getting keytab entry for '%s'\n",
 					 k5err, spn);
-				krb5_free_string(context, k5err);
+				free(k5err);
 				k5err = NULL;
 				/*
 				 * We tried the active directory machine account
@@ -986,7 +986,7 @@  out:
 		k5_free_default_realm(context, default_realm);
 	if (realmnames)
 		krb5_free_host_realm(context, realmnames);
-	krb5_free_string(context, k5err);
+	free(k5err);
 	return retval;
 }
 
@@ -1355,7 +1355,7 @@  out_free_kt:
 out_free_context:
 	krb5_free_context(context);
 out:
-	krb5_free_string(context, k5err);
+	free(k5err);
 	return retval;
 }