Fix kadm5 error cleanup

lib/kadm5/chpass_s.c

@@ -64,8 +64,8 @@ change(void *server_handle,
 
     ret = context->db->hdb_fetch_kvno(context->context, context->db, princ,
 				      HDB_F_DECRYPT|HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent);
-    if(ret)
+    if (ret)
-	goto out;
+	goto out2;
 
     if (keepold || cond) {
 	/*
@@ -74,14 +74,14 @@ change(void *server_handle,
 	 */
 	ret = hdb_add_current_keys_to_history(context->context, &ent.entry);
 	if (ret)
-	    goto out;
+	    goto out3;
     }
 
     if (context->db->hdb_capability_flags & HDB_CAP_F_HANDLE_PASSWORDS) {
 	ret = context->db->hdb_password(context->context, context->db,
 					&ent, password, cond);
 	if (ret)
-	    goto out2;
+	    goto out3;
     } else {
 
 	num_keys = ent.entry.keys.len;
@@ -94,7 +94,7 @@ change(void *server_handle,
 			      password);
 	if(ret) {
 	    _kadm5_free_keys(context->context, num_keys, keys);
-	    goto out2;
+	    goto out3;
 	}
 	_kadm5_free_keys(context->context, num_keys, keys);
 
@@ -112,7 +112,7 @@ change(void *server_handle,
 	    ret = KADM5_PASS_REUSE;
 	    krb5_set_error_message(context->context, ret,
 				   "Password reuse forbidden");
-	    goto out2;
+	    goto out3;
 	}
     }
     ent.entry.kvno++;
@@ -127,20 +127,20 @@ change(void *server_handle,
 	ext.data.element = choice_HDB_extension_data_hist_keys;
 	ret = hdb_replace_extension(context->context, &ent.entry, &ext);
 	if (ret)
-	    goto out2;
+	    goto out3;
     }
 
     ret = hdb_seal_keys(context->context, context->db, &ent.entry);
     if (ret)
-        goto out2;
+        goto out3;
 
     ret = _kadm5_set_modifier(context, &ent.entry);
     if(ret)
-	goto out2;
+	goto out3;
 
     ret = _kadm5_bump_pw_expire(context, &ent.entry);
     if (ret)
-	goto out2;
+	goto out3;
 
     /* This logs the change for iprop and writes to the HDB */
     ret = kadm5_log_modify(context, &ent.entry,
@@ -149,10 +149,11 @@ change(void *server_handle,
                            KADM5_KEY_DATA | KADM5_KVNO |
                            KADM5_PW_EXPIRATION | KADM5_TL_DATA);
 
-out2:
+ out3:
     hdb_free_entry(context->context, &ent);
-out:
+ out2:
     (void) kadm5_log_end(context);
+ out:
     if (!context->keep_open) {
         kadm5_ret_t ret2;
         ret2 = context->db->hdb_close(context->context, context->db);
@@ -221,28 +222,28 @@ kadm5_s_chpass_principal_with_key(void *server_handle,
 
     ret = context->db->hdb_fetch_kvno(context->context, context->db, princ, 0,
 				      HDB_F_GET_ANY|HDB_F_ADMIN_DATA, &ent);
-    if(ret == HDB_ERR_NOENTRY)
+    if (ret == HDB_ERR_NOENTRY)
-	goto out;
+	goto out2;
     if (keepold) {
 	ret = hdb_add_current_keys_to_history(context->context, &ent.entry);
 	if (ret)
-	    goto out2;
+	    goto out3;
     }
     ret = _kadm5_set_keys2(context, &ent.entry, n_key_data, key_data);
-    if(ret)
+    if (ret)
-	goto out2;
+	goto out3;
     ent.entry.kvno++;
     ret = _kadm5_set_modifier(context, &ent.entry);
-    if(ret)
+    if (ret)
-	goto out2;
+	goto out3;
     ret = _kadm5_bump_pw_expire(context, &ent.entry);
     if (ret)
-	goto out2;
+	goto out3;
 
     if (keepold) {
 	ret = hdb_seal_keys(context->context, context->db, &ent.entry);
 	if (ret)
-	    goto out2;
+	    goto out3;
     } else {
 	HDB_extension ext;
 
@@ -260,10 +261,11 @@ kadm5_s_chpass_principal_with_key(void *server_handle,
                            KADM5_MOD_TIME | KADM5_KEY_DATA | KADM5_KVNO |
                            KADM5_PW_EXPIRATION | KADM5_TL_DATA);
 
-out2:
+ out3:
     hdb_free_entry(context->context, &ent);
-out:
+ out2:
     (void) kadm5_log_end(context);
+ out:
     if (!context->keep_open) {
         kadm5_ret_t ret2;
         ret2 = context->db->hdb_close(context->context, context->db);

lib/kadm5/context_s.c

@@ -57,8 +57,21 @@ kadm5_s_lock(void *server_handle)
 	return ret;
 
     ret = context->db->hdb_lock(context->context, context->db, HDB_WLOCK);
-    if (ret)
+    if (ret) {
+        (void) context->db->hdb_close(context->context, context->db);
 	return ret;
+    }
+
+    /*
+     * Attempt to recover the log.  This will generally fail on slaves,
+     * and we can't tell if we're on a slave here.
+     *
+     * Perhaps we could set a flag in the kadm5_server_context to
+     * indicate whether a read has been done without recovering the log,
+     * in which case we could fail any subsequent writes.
+     */
+    if (kadm5_log_init(context) == 0)
+        (void) kadm5_log_end(context);
 
     context->keep_open = 1;
     return 0;

lib/kadm5/create_s.c

@@ -124,13 +124,15 @@ kadm5_s_create_principal_with_key(void *server_handle,
 			   | KADM5_AUX_ATTRIBUTES
 			   | KADM5_POLICY_CLR | KADM5_LAST_SUCCESS
 			   | KADM5_LAST_FAILED | KADM5_FAIL_AUTH_COUNT);
-    if(ret)
+    if (ret)
-	goto out;
+        return ret;
 
     if (!context->keep_open) {
         ret = context->db->hdb_open(context->context, context->db, O_RDWR, 0);
-        if (ret)
+        if (ret) {
-            goto out;
+            hdb_free_entry(context->context, &ent);
+            return ret;
+        }
     }
 
     ret = kadm5_log_init(context);
@@ -139,13 +141,14 @@ kadm5_s_create_principal_with_key(void *server_handle,
 
     ret = hdb_seal_keys(context->context, context->db, &ent.entry);
     if (ret)
-	goto out;
+	goto out2;
 
     /* This logs the change for iprop and writes to the HDB */
     ret = kadm5_log_create(context, &ent.entry);
 
-out:
+ out2:
     (void) kadm5_log_end(context);
+ out:
     if (!context->keep_open) {
         kadm5_ret_t ret2;
         ret2 = context->db->hdb_close(context->context, context->db);
@@ -183,12 +186,14 @@ kadm5_s_create_principal(void *server_handle,
 			   | KADM5_POLICY_CLR | KADM5_LAST_SUCCESS
 			   | KADM5_LAST_FAILED | KADM5_FAIL_AUTH_COUNT);
     if (ret)
-	goto out;
+        return ret;
 
     if (!context->keep_open) {
         ret = context->db->hdb_open(context->context, context->db, O_RDWR, 0);
-        if (ret)
+        if (ret) {
-            goto out;
+            hdb_free_entry(context->context, &ent);
+            return ret;
+        }
     }
 
     ret = kadm5_log_init(context);
@@ -200,17 +205,18 @@ kadm5_s_create_principal(void *server_handle,
 
     ret = _kadm5_set_keys(context, &ent.entry, n_ks_tuple, ks_tuple, password);
     if (ret)
-	goto out;
+	goto out2;
 
     ret = hdb_seal_keys(context->context, context->db, &ent.entry);
     if (ret)
-	goto out;
+	goto out2;
 
     /* This logs the change for iprop and writes to the HDB */
     ret = kadm5_log_create(context, &ent.entry);
 
- out:
+ out2:
     (void) kadm5_log_end(context);
+ out:
     if (!context->keep_open) {
         kadm5_ret_t ret2;
         ret2 = context->db->hdb_close(context->context, context->db);

lib/kadm5/delete_s.c

@@ -58,23 +58,24 @@ kadm5_s_delete_principal(void *server_handle, krb5_principal princ)
     ret = context->db->hdb_fetch_kvno(context->context, context->db, princ,
 				      HDB_F_DECRYPT|HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent);
     if (ret == HDB_ERR_NOENTRY)
-	goto out;
+	goto out2;
     if (ent.entry.flags.immutable) {
 	ret = KADM5_PROTECT_PRINCIPAL;
-	goto out2;
+	goto out3;
     }
 
     ret = hdb_seal_keys(context->context, context->db, &ent.entry);
     if (ret)
-	goto out2;
+	goto out3;
 
     /* This logs the change for iprop and writes to the HDB */
     ret = kadm5_log_delete(context, princ);
 
-out2:
+ out3:
     hdb_free_entry(context->context, &ent);
-out:
+ out2:
     (void) kadm5_log_end(context);
+ out:
     if (!context->keep_open) {
         kadm5_ret_t ret2;
         ret2 = context->db->hdb_close(context->context, context->db);

lib/kadm5/free.c

@@ -75,10 +75,8 @@ kadm5_free_principal_ent(void *server_handle,
 	free(tp->tl_data_contents);
 	free(tp);
     }
-    if (princ->key_data != NULL)
+    free(princ->key_data);
-	free(princ->key_data);
+    free(princ->policy);
-    if (princ->policy)
-        free(princ->policy);
 }
 
 void

lib/kadm5/get_princs_s.c

@@ -88,7 +88,7 @@ kadm5_s_get_principals(void *server_handle,
 
     if (!context->keep_open) {
 	ret = context->db->hdb_open(context->context, context->db, O_RDONLY, 0);
-	if(ret) {
+	if (ret) {
 	    krb5_warn(context->context, ret, "opening database");
 	    return ret;
 	}
@@ -102,21 +102,24 @@ kadm5_s_get_principals(void *server_handle,
 	aret = asprintf(&d.exp2, "%s@%s", expression, r);
 	free(r);
 	if (aret == -1 || d.exp2 == NULL) {
-	    return ENOMEM;
+	    ret = ENOMEM;
+            goto out;
 	}
     }
     d.princs = NULL;
     d.count = 0;
     ret = hdb_foreach(context->context, context->db, HDB_F_ADMIN_DATA, foreach, &d);
-    if (!context->keep_open)
+
-	context->db->hdb_close(context->context, context->db);
+    if (ret == 0)
-    if(ret == 0)
 	ret = add_princ(&d, NULL);
-    if(ret == 0){
+    if (ret == 0){
 	*princs = d.princs;
 	*count = d.count - 1;
-    }else
+    } else
 	kadm5_free_name_list(context, d.princs, &d.count);
     free(d.exp2);
+ out:
+    if (!context->keep_open)
+	context->db->hdb_close(context->context, context->db);
     return _kadm5_error_code(ret);
 }

lib/kadm5/get_s.c

@@ -126,6 +126,7 @@ kadm5_s_get_principal(void *server_handle,
     int hdb_is_rw = 1;
 
     memset(&ent, 0, sizeof(ent));
+    memset(out, 0, sizeof(*out));
 
     if (!context->keep_open) {
 	ret = context->db->hdb_open(context->context, context->db, O_RDWR, 0);
@@ -133,7 +134,7 @@ kadm5_s_get_principal(void *server_handle,
             ret = context->db->hdb_open(context->context, context->db, O_RDONLY, 0);
             hdb_is_rw = 0;
         }
-	if(ret)
+	if (ret)
 	    return ret;
     }
 
@@ -145,21 +146,18 @@ kadm5_s_get_principal(void *server_handle,
      * indicate whether a read has been done without recovering the log,
      * in which case we could fail any subsequent writes.
      */
-    if (hdb_is_rw)
+    if (hdb_is_rw && kadm5_log_init_nb(context) == 0)
-        (void) kadm5_log_init_nb(context);
+        (void) kadm5_log_end(context);
 
     ret = context->db->hdb_fetch_kvno(context->context, context->db, princ,
 				      HDB_F_DECRYPT|HDB_F_ALL_KVNOS|
 				      HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent);
-    if (hdb_is_rw)
-        kadm5_log_end(context);
 
     if (!context->keep_open)
 	context->db->hdb_close(context->context, context->db);
     if(ret)
 	return _kadm5_error_code(ret);
 
-    memset(out, 0, sizeof(*out));
     if(mask & KADM5_PRINCIPAL)
 	ret  = krb5_copy_principal(context->context, ent.entry.principal,
 				   &out->principal);
@@ -296,10 +294,8 @@ kadm5_s_get_principal(void *server_handle,
 	krb5_free_salt(context->context, salt);
 	assert( out->n_key_data == n_keys );
     }
-    if(ret){
+    if (ret)
-	kadm5_free_principal_ent(context, out);
 	goto out;
-    }
     if(mask & KADM5_TL_DATA) {
 	time_t last_pw_expire;
 	const HDB_Ext_PKINIT_acl *acl;
@@ -311,15 +307,13 @@ kadm5_s_get_principal(void *server_handle,
 	    _krb5_put_int(buf, last_pw_expire, sizeof(buf));
 	    ret = add_tl_data(out, KRB5_TL_LAST_PWD_CHANGE, buf, sizeof(buf));
 	}
-	if(ret){
+	if (ret)
-	    kadm5_free_principal_ent(context, out);
 	    goto out;
-	}
 	/*
 	 * If the client was allowed to get key data, let it have the
 	 * password too.
 	 */
-	if(mask & KADM5_KEY_DATA) {
+	if (mask & KADM5_KEY_DATA) {
 	    heim_utf8_string pw;
 
 	    ret = hdb_entry_get_password(context->context,
@@ -338,24 +332,18 @@ kadm5_s_get_principal(void *server_handle,
 
 	    ASN1_MALLOC_ENCODE(HDB_Ext_PKINIT_acl, buf.data, buf.length,
 				acl, &len, ret);
-	    if (ret) {
+	    if (ret)
-		kadm5_free_principal_ent(context, out);
 		goto out;
-	    }
 	    if (len != buf.length)
 		krb5_abortx(context->context,
 			    "internal ASN.1 encoder error");
 	    ret = add_tl_data(out, KRB5_TL_PKINIT_ACL, buf.data, buf.length);
 	    free(buf.data);
-	    if (ret) {
+	    if (ret)
-		kadm5_free_principal_ent(context, out);
 		goto out;
-	    }
 	}
-	if(ret){
+	if (ret)
-	    kadm5_free_principal_ent(context, out);
 	    goto out;
-	}
 
 	ret = hdb_entry_get_aliases(&ent.entry, &aliases);
 	if (ret == 0 && aliases) {
@@ -364,27 +352,23 @@ kadm5_s_get_principal(void *server_handle,
 
 	    ASN1_MALLOC_ENCODE(HDB_Ext_Aliases, buf.data, buf.length,
 			       aliases, &len, ret);
-	    if (ret) {
+	    if (ret)
-		kadm5_free_principal_ent(context, out);
 		goto out;
-	    }
 	    if (len != buf.length)
 		krb5_abortx(context->context,
 			    "internal ASN.1 encoder error");
 	    ret = add_tl_data(out, KRB5_TL_ALIASES, buf.data, buf.length);
 	    free(buf.data);
-	    if (ret) {
+	    if (ret)
-		kadm5_free_principal_ent(context, out);
 		goto out;
-	    }
 	}
-	if(ret){
+	if (ret)
-	    kadm5_free_principal_ent(context, out);
 	    goto out;
-	}
-
     }
-out:
+
+ out:
+    if (ret)
+        kadm5_free_principal_ent(context, out);
     hdb_free_entry(context->context, &ent);
 
     return _kadm5_error_code(ret);

lib/kadm5/modify_s.c

@@ -64,14 +64,14 @@ modify_principal(void *server_handle,
 
     ret = context->db->hdb_fetch_kvno(context->context, context->db,
 				      princ->principal, HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent);
-    if(ret)
+    if (ret)
-	goto out;
+	goto out2;
     ret = _kadm5_setup_entry(context, &ent, mask, princ, mask, NULL, 0);
-    if(ret)
+    if (ret)
-	goto out2;
+	goto out3;
     ret = _kadm5_set_modifier(context, &ent.entry);
-    if(ret)
+    if (ret)
-	goto out2;
+	goto out3;
 
     /*
      * If any keys are bogus, disallow the modify.  If the keys were
@@ -84,12 +84,12 @@ modify_principal(void *server_handle,
     if ((mask & KADM5_KEY_DATA) &&
 	kadm5_some_keys_are_bogus(princ->n_key_data, princ->key_data)) {
 	ret = KADM5_AUTH_GET_KEYS; /* Not quite appropriate, but it'll do */
-	goto out2;
+	goto out3;
     }
 
     ret = hdb_seal_keys(context->context, context->db, &ent.entry);
     if (ret)
-	goto out2;
+	goto out3;
 
     if ((mask & KADM5_POLICY)) {
 	HDB_extension ext;
@@ -101,23 +101,24 @@ modify_principal(void *server_handle,
 	ext.data.u.policy = strdup(princ->policy);
 	if (ext.data.u.policy == NULL) {
 	    ret = ENOMEM;
-	    goto out2;
+	    goto out3;
 	}
 	/* This calls free_HDB_extension(), freeing ext.data.u.policy */
 	ret = hdb_replace_extension(context->context, &ent.entry, &ext);
         free(ext.data.u.policy);
 	if (ret)
-	    goto out2;
+	    goto out3;
     }
 
     /* This logs the change for iprop and writes to the HDB */
     ret = kadm5_log_modify(context, &ent.entry,
                            mask | KADM5_MOD_NAME | KADM5_MOD_TIME);
 
-out2:
+ out3:
     hdb_free_entry(context->context, &ent);
-out:
+ out2:
     (void) kadm5_log_end(context);
+ out:
     if (!context->keep_open) {
         kadm5_ret_t ret2;
         ret2 = context->db->hdb_close(context->context, context->db);

lib/kadm5/randkey_s.c

@@ -67,33 +67,33 @@ kadm5_s_randkey_principal(void *server_handle,
     ret = context->db->hdb_fetch_kvno(context->context, context->db, princ,
 				      HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent);
     if(ret)
-	goto out;
+	goto out2;
 
     if (keepold) {
 	ret = hdb_add_current_keys_to_history(context->context, &ent.entry);
 	if (ret)
-	    goto out2;
+	    goto out3;
     }
 
     ret = _kadm5_set_keys_randomly(context, &ent.entry, n_ks_tuple, ks_tuple,
                                    new_keys, n_keys);
     if (ret)
-	goto out2;
+	goto out3;
     ent.entry.kvno++;
 
     ent.entry.flags.require_pwchange = 0;
 
     ret = _kadm5_set_modifier(context, &ent.entry);
     if(ret)
-	goto out3;
+	goto out4;
     ret = _kadm5_bump_pw_expire(context, &ent.entry);
     if (ret)
-	goto out2;
+	goto out4;
 
     if (keepold) {
 	ret = hdb_seal_keys(context->context, context->db, &ent.entry);
 	if (ret)
-	    goto out2;
+	    goto out4;
     } else {
 	HDB_extension ext;
 
@@ -112,7 +112,7 @@ kadm5_s_randkey_principal(void *server_handle,
                            KADM5_KEY_DATA | KADM5_KVNO |
                            KADM5_PW_EXPIRATION | KADM5_TL_DATA);
 
-out3:
+ out4:
     if (ret) {
 	int i;
 
@@ -122,10 +122,11 @@ out3:
 	*new_keys = NULL;
 	*n_keys = 0;
     }
-out2:
+ out3:
     hdb_free_entry(context->context, &ent);
-out:
+ out2:
     (void) kadm5_log_end(context);
+ out:
     if (!context->keep_open) {
         kadm5_ret_t ret2;
         ret2 = context->db->hdb_close(context->context, context->db);

lib/kadm5/rename_s.c

@@ -46,7 +46,7 @@ kadm5_s_rename_principal(void *server_handle,
     krb5_principal oldname;
 
     memset(&ent, 0, sizeof(ent));
-    if(krb5_principal_compare(context->context, source, target))
+    if (krb5_principal_compare(context->context, source, target))
 	return KADM5_DUP; /* XXX is this right? */
     if (!context->keep_open) {
 	ret = context->db->hdb_open(context->context, context->db, O_RDWR, 0);
@@ -61,11 +61,11 @@ kadm5_s_rename_principal(void *server_handle,
     ret = context->db->hdb_fetch_kvno(context->context, context->db,
 				      source, HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent);
     if (ret)
-	goto out;
+	goto out2;
     oldname = ent.entry.principal;
     ret = _kadm5_set_modifier(context, &ent.entry);
     if (ret)
-	goto out2;
+	goto out3;
     {
 	/* fix salt */
 	size_t i;
@@ -79,33 +79,35 @@ kadm5_s_rename_principal(void *server_handle,
 	    if(ent.entry.keys.val[i].salt == NULL){
 		ent.entry.keys.val[i].salt =
 		    malloc(sizeof(*ent.entry.keys.val[i].salt));
-		if(ent.entry.keys.val[i].salt == NULL)
+		if (ent.entry.keys.val[i].salt == NULL)
-		    return ENOMEM;
+		    ret = ENOMEM;
-		ret = copy_Salt(&salt, ent.entry.keys.val[i].salt);
+                else
-		if(ret)
+                    ret = copy_Salt(&salt, ent.entry.keys.val[i].salt);
+		if (ret)
 		    break;
 	    }
 	}
 	krb5_free_salt(context->context, salt2);
     }
     if (ret)
-	goto out2;
+	goto out3;
 
     /* Borrow target */
     ent.entry.principal = target;
     ret = hdb_seal_keys(context->context, context->db, &ent.entry);
     if (ret)
-	goto out2;
+	goto out3;
 
     /* This logs the change for iprop and writes to the HDB */
     ret = kadm5_log_rename(context, source, &ent.entry);
 
-out2:
+ out3:
     ent.entry.principal = oldname; /* Unborrow target */
     hdb_free_entry(context->context, &ent);
 
-out:
+ out2:
     (void) kadm5_log_end(context);
+ out:
     if (!context->keep_open) {
         kadm5_ret_t ret2;
         ret2 = context->db->hdb_close(context->context, context->db);

lib/kadm5/setkey3_s.c

@@ -33,8 +33,6 @@
 
 #include "kadm5_locl.h"
 
-RCSID("$Id$");
-
 /**
  * Server-side function to set new keys for a principal.
  */
@@ -65,10 +63,16 @@ kadm5_s_setkey_principal_3(void *server_handle,
 
     ret = context->db->hdb_fetch_kvno(context->context, context->db, princ,
 				      HDB_F_GET_ANY|HDB_F_ADMIN_DATA, 0, &ent);
+    if (ret) {
+        (void) kadm5_log_end(context);
+        if (!context->keep_open)
+            context->db->hdb_close(context->context, context->db);
+        return ret;
+    }
+
     if (keepold) {
-	if (ret == 0)
+        ret = hdb_add_current_keys_to_history(context->context, &ent.entry);
-	    ret = hdb_add_current_keys_to_history(context->context, &ent.entry);
+    } else
-    } else if (ret == 0)
 	ret = hdb_clear_extension(context->context, &ent.entry,
 				  choice_HDB_extension_data_hist_keys);