r1192: Fixed all memleaks/error code return path leaks I can find. Not sure if compiles yet,

but will soon :-).
Jeremy.
(This used to be commit 0d982956f6ba2f284ffa4313a9e7581a79dbf397)

1 files changed, 94 insertions, 42 deletions

diff --git a/source3/libads/kerberos_keytab.c b/source3/libads/kerberos_keytab.c
index ec44bfbe68..95ff0c1cf3 100644
--- a/source3/libads/kerberos_keytab.c
+++ b/source3/libads/kerberos_keytab.c
@@ -46,7 +46,7 @@ void name_to_fqdn(fstring fqdn, const char *name)
 }
 
 /**********************************************************************
-  Adds a single service principal, i.e. 'host' to the system keytab
+ Adds a single service principal, i.e. 'host' to the system keytab
 ***********************************************************************/
 
 int ads_keytab_add_entry(const char *srvPrinc, ADS_STRUCT *ads)
@@ -290,7 +290,7 @@ out:
 }
 
 /**********************************************************************
-  Flushes all entries from the system keytab.
+ Flushes all entries from the system keytab.
 ***********************************************************************/
 
 int ads_keytab_flush(ADS_STRUCT *ads)
@@ -339,35 +339,40 @@ int ads_keytab_flush(ADS_STRUCT *ads)
 		goto out;
 	}
 
-HERE
-
 	ret = krb5_kt_start_seq_get(context, keytab, &cursor);
 	if (ret != KRB5_KT_END && ret != ENOENT) {
-		while (!krb5_kt_next_entry(context, keytab, &entry, &cursor)) {
+		while (!krb5_kt_next_entry(context, keytab, &kt_entry, &cursor)) {
 			ret = krb5_kt_end_seq_get(context, keytab, &cursor);
+			cursor = NULL;
 			if (ret) {
-				DEBUG(1,("krb5_kt_end_seq_get() failed (%s)\n",error_message(ret)));
+				DEBUG(1,("ads_keytab_flush: krb5_kt_end_seq_get() failed (%s)\n",error_message(ret)));
 				goto out;
 			}
-			ret = krb5_kt_remove_entry(context, keytab, &entry);
+			ret = krb5_kt_remove_entry(context, keytab, &kt_entry);
 			if (ret) {
-				DEBUG(1,("krb5_kt_remove_entry failed (%s)\n",error_message(ret)));
+				DEBUG(1,("ads_keytab_flush: krb5_kt_remove_entry failed (%s)\n",error_message(ret)));
 				goto out;
 			}
 			ret = krb5_kt_start_seq_get(context, keytab, &cursor);
 			if (ret) {
-				DEBUG(1,("krb5_kt_start_seq failed (%s)\n",error_message(ret)));
+				DEBUG(1,("ads_keytab_flush: krb5_kt_start_seq failed (%s)\n",error_message(ret)));
 				goto out;
 			}
-			ret = krb5_kt_free_entry(context, &entry);
+			ret = krb5_kt_free_entry(context, &kt_entry);
+			ZERO_STRUCT(kt_entry);
 			if (ret) {
-				DEBUG(1,("krb5_kt_remove_entry failed (%s)\n",error_message(ret)));
+				DEBUG(1,("ads_keytab_flush: krb5_kt_remove_entry failed (%s)\n",error_message(ret)));
 				goto out;
 			}
 		}
 	}
+
+	/* Ensure we don't double free. */
+	ZERO_STRUCT(kt_entry);
+	cursor = NULL;
+
 	if (!ADS_ERR_OK(ads_clear_spns(ads, global_myname()))) {
-		DEBUG(1,("Error while clearing service principal listings in LDAP.\n"));
+		DEBUG(1,("ads_keytab_flush: Error while clearing service principal listings in LDAP.\n"));
 		goto out;
 	}
 
@@ -392,78 +397,105 @@ out:
 	return ret;
 }
 
+/**********************************************************************
+ Adds all the required service principals to the system keytab.
+***********************************************************************/
 
 int ads_keytab_create_default(ADS_STRUCT *ads)
 {
-	krb5_error_code ret;
-	krb5_context context;
-	krb5_keytab keytab;
-	krb5_kt_cursor cursor;
-	krb5_keytab_entry entry;
+	krb5_error_code ret = 0;
+	krb5_context context = NULL;
+	krb5_keytab keytab = NULL;
+	krb5_kt_cursor cursor = NULL;
+	krb5_keytab_entry kt_entry;
 	krb5_kvno kvno;
-	char *ktprinc;
 	int i, found = 0;
-	char **oldEntries;
+	char **oldEntries = NULL;
 
 	ret = ads_keytab_add_entry("host", ads);
 	if (ret) {
-		DEBUG(1,("ads_keytab_add_entry failed while adding 'host'.\n"));
+		DEBUG(1,("ads_keytab_create_default: ads_keytab_add_entry failed while adding 'host'.\n"));
 		return ret;
 	}
 	ret = ads_keytab_add_entry("cifs", ads);
 	if (ret) {
-		DEBUG(1,("ads_keytab_add_entry failed while adding 'cifs'.\n"));
+		DEBUG(1,("ads_keytab_create_default: ads_keytab_add_entry failed while adding 'cifs'.\n"));
 		return ret;
 	}
 
 	kvno = (krb5_kvno) ads_get_kvno(ads, global_myname());
 	if (kvno == -1) {
-		DEBUG(1,("ads_get_kvno failed to determine the system's kvno.\n"));
+		DEBUG(1,("ads_keytab_create_default: ads_get_kvno failed to determine the system's kvno.\n"));
 		return -1;
 	}
 
-	DEBUG(1,("Searching for keytab entries to preserve and update.\n"));
+	DEBUG(3,("ads_keytab_create_default: Searching for keytab entries to preserve and update.\n"));
 	/* Now loop through the keytab and update any other existing entries... */
+
+	ZERO_STRUCT(kt_entry);
+
 	initialize_krb5_error_table();
 	ret = krb5_init_context(&context);
 	if (ret) {
-		DEBUG(1,("could not krb5_init_context: %s\n",error_message(ret)));
+		DEBUG(1,("ads_keytab_create_default: could not krb5_init_context: %s\n",error_message(ret)));
 		return ret;
 	}
 	ret = krb5_kt_default(context, &keytab);
 	if (ret) {
-		DEBUG(1,("krb5_kt_default failed (%s)\n",error_message(ret)));
-		return ret;
+		DEBUG(1,("ads_keytab_create_default: krb5_kt_default failed (%s)\n",error_message(ret)));
+		goto done;
 	}
 
 	ret = krb5_kt_start_seq_get(context, keytab, &cursor);
 	if (ret != KRB5_KT_END && ret != ENOENT ) {
-		while ((ret = krb5_kt_next_entry(context, keytab, &entry, &cursor)) == 0) {
+		while ((ret = krb5_kt_next_entry(context, keytab, &kt_entry, &cursor)) == 0) {
+			krb5_kt_free_entry(context, &kt_entry);
+			ZERO_STRUCT(kt_entry);
 			found++;
 		}
 	}
+	krb5_kt_end_seq_get(context, keytab, &cursor);
+	cursor = NULL;
 
-	DEBUG(1, ("Found %d entries in the keytab.\n", found));
+	/*
+	 * Hmmm. There is no "rewind" function for the keytab. This means we have a race condition
+	 * where someone else could add entries after we've counted them. Re-open asap to minimise
+	 * the race. JRA.
+	 */
+	
+	DEBUG(3, ("ads_keytab_create_default: Found %d entries in the keytab.\n", found));
 	if (!found) {
 		goto done;
 	}
 	oldEntries = (char **) malloc(found * sizeof(char *));
 	if (!oldEntries) {
-		DEBUG(1,("Failed to allocate space to store the old keytab entries (malloc failed?).\n"));
-		return ENOMEM;
+		DEBUG(1,("ads_keytab_create_default: Failed to allocate space to store the old keytab entries (malloc failed?).\n"));
+		ret = -1;
+		goto done;
 	}
-	memset(oldEntries, 0, found * sizeof(char *));
+	memset(oldEntries, '\0', found * sizeof(char *));
 
 	ret = krb5_kt_start_seq_get(context, keytab, &cursor);
 	if (ret != KRB5_KT_END && ret != ENOENT ) {
-		while ((ret = krb5_kt_next_entry(context, keytab, &entry, &cursor)) == 0) {
-			if (entry.vno != kvno) {
-				krb5_unparse_name(context, entry.principal, &ktprinc);
-				for (i = 0; *(ktprinc + i); i++) {
-					if (*(ktprinc + i) == '/') {
-						*(ktprinc + i) = (char) NULL;
-						break;
-					}
+		while ((ret = krb5_kt_next_entry(context, keytab, &kt_entry, &cursor)) == 0) {
+			if (kt_entry.vno != kvno) {
+				char *ktprinc;
+				char *p;
+
+				/* This returns a malloc'ed string in ktprinc. */
+				ret = krb5_unparse_name(context, kt_entry.principal, &ktprinc);
+				if (ret) {
+					DEBUG(1,("krb5_unparse_name failed (%s)\n", error_message(ret)));
+					goto out;
+				}
+				/*
+				 * From looking at the krb5 source they don't seem to take locale
+				 * or mb strings into account. Maybe this is because they assume utf8 ?
+				 * In this case we may need to convert from utf8 to mb charset here ? JRA.
+				 */
+				p = strchr_m(ktprinc, '/');
+				if (p) {
+					*p = '\0';
 				}
 				for (i = 0; i < found; i++) {
 					if (!oldEntries[i]) {
@@ -475,17 +507,37 @@ int ads_keytab_create_default(ADS_STRUCT *ads)
 					}
 				}
 			}
+			krb5_kt_free_entry(context, &kt_entry);
+			ZERO_STRUCT(kt_entry);
 		}
 		for (i = 0; oldEntries[i]; i++) {
 			ret |= ads_keytab_add_entry(oldEntries[i], ads);
-			free(oldEntries[i]);
+			SAFE_FREE(oldEntries[i]);
 		}
+		krb5_kt_end_seq_get(context, keytab, &cursor);
 	}
-	free(oldEntries);
+	cursor = NULL;
 
 done:
 
-	krb5_kt_close(context, keytab);
+	SAFE_FREE(oldEntries);
+
+	{
+		krb5_keytab_entry zero_kt_entry;
+		ZERO_STRUCT(zero_kt_entry);
+		if (memcmp(&zero_kt_entry, &kt_entry, sizeof(krb5_keytab_entry))) {
+			krb5_kt_free_entry(context, &kt_entry);
+		}
+	}
+	if (cursor && keytab) {
+		krb5_kt_end_seq_get(context, keytab, &cursor);	
+	}
+	if (keytab) {
+		krb5_kt_close(context, keytab);
+	}
+	if (context) {
+		krb5_free_context(context);
+	}
 	return ret;
 }
 #endif /* HAVE_KRB5 */