Zero length fields when freeing object contents

In krb5_free_data_contents() and krb5_free_checksum_contents(), zero
the length as well as the data pointer to leave the object in a valid
state.  Add asserts to existing test harnesses to verify the new
behavior.

In the krb5 GSS mech's kg_checksum_channel_bindings(), remove the code
to reallocate the checksum with xmalloc(), as it relied on
krb5_free_checksum_contents() leaving the object in an invalid state.
This code was added in commit a30fb4c4400f13a2690df7ef910b7ac0ccbcf194
to match an xfree() call, but commit
29337e7c7b796685fb6a03466d32147e17aa2d16 replaced that xfree() with a
krb5_free_checksum_contents().  (In addition, the xmalloc and xfree
wrappers never evolved to do anything beyond malloc and free.)

In kpropd's recv_database(), don't free outbuf until we are done using
its length.

[ghudson@mit.edu: rewrote commit message; edited doxygen comment
changes to mention version]

ticket: 8871 (new)

diff --git a/src/include/krb5/krb5.hin b/src/include/krb5/krb5.hin
index d4868535779938328485fb3f8fe187417fdcca40..4cd9ad5ffe8aac9b1dedf412e16d5f1788857104 100644 (file)
--- a/src/include/krb5/krb5.hin
+++ b/src/include/krb5/krb5.hin
@@ -4692,6 +4692,8 @@ krb5_free_checksum(krb5_context context, krb5_checksum *val);
  * @param [in] val              Checksum structure to free contents of
  *
  * This function frees the contents of @a val, but not the structure itself.
+ * It sets the checksum's data pointer to null and (beginning in release 1.19)
+ * sets its length to zero.
  */
 void KRB5_CALLCONV
 krb5_free_checksum_contents(krb5_context context, krb5_checksum *val);
@@ -4751,6 +4753,8 @@ krb5_free_octet_data(krb5_context context, krb5_octet_data *val);
  * @param [in] val              Data structure to free contents of
  *
  * This function frees the contents of @a val, but not the structure itself.
+ * It sets the structure's data pointer to null and (beginning in release 1.19)
+ * sets its length to zero.
  */
 void KRB5_CALLCONV
 krb5_free_data_contents(krb5_context context, krb5_data *val);

diff --git a/src/kprop/kpropd.c b/src/kprop/kpropd.c
index 5622d56e140a2f666ad20f077555cb023b407770..ab4a764aa57cdd20299f8445137cc73d39d9514a 100644 (file)
--- a/src/kprop/kpropd.c
+++ b/src/kprop/kpropd.c
@@ -1412,7 +1412,6 @@ recv_database(krb5_context context, int fd, int database_fd,
         }
         n = write(database_fd, outbuf.data, outbuf.length);
         krb5_free_data_contents(context, &inbuf);
-        krb5_free_data_contents(context, &outbuf);
         if (n < 0) {
             snprintf(buf, sizeof(buf),
                      "while writing database block starting at offset %d",
@@ -1426,6 +1425,7 @@ recv_database(krb5_context context, int fd, int database_fd,
             send_error(context, fd, KRB5KRB_ERR_GENERIC, buf);
         }
         received_size += outbuf.length;
+        krb5_free_data_contents(context, &outbuf);
     }
 
     /* OK, we've seen the entire file.  Did we get too many bytes? */

diff --git a/src/lib/crypto/crypto_tests/t_cksums.c b/src/lib/crypto/crypto_tests/t_cksums.c
index 4da14ea4382984564601123f507f193edb00c415..8297fcbf5ce950a693592eb4920a9d9417640f4b 100644 (file)
--- a/src/lib/crypto/crypto_tests/t_cksums.c
+++ b/src/lib/crypto/crypto_tests/t_cksums.c
@@ -254,6 +254,7 @@ main(int argc, char **argv)
         }
 
         krb5_free_checksum_contents(context, &cksum);
+        assert(cksum.length == 0);
     }
     return status;
 }

diff --git a/src/lib/gssapi/krb5/util_cksum.c b/src/lib/gssapi/krb5/util_cksum.c
index a1770774e1c84b4eacaea6d061d20f4e342643ec..2d6b50b2a29ffa86922bffd6057b3e7869e981f0 100644 (file)
--- a/src/lib/gssapi/krb5/util_cksum.c
+++ b/src/lib/gssapi/krb5/util_cksum.c
@@ -39,7 +39,6 @@ kg_checksum_channel_bindings(context, cb, cksum)
     size_t sumlen;
     krb5_data plaind;
     krb5_error_code code;
-    void *temp;
 
     /* initialize the the cksum */
     code = krb5_c_checksum_length(context, CKSUMTYPE_RSA_MD5, &sumlen);
@@ -88,21 +87,6 @@ kg_checksum_channel_bindings(context, cb, cksum)
 
     code = krb5_c_make_checksum(context, CKSUMTYPE_RSA_MD5, 0, 0,
                                 &plaind, cksum);
-    if (code)
-        goto cleanup;
-
-    if ((temp = xmalloc(cksum->length)) == NULL) {
-        krb5_free_checksum_contents(context, cksum);
-        code = ENOMEM;
-        goto cleanup;
-    }
-
-    memcpy(temp, cksum->contents, cksum->length);
-    krb5_free_checksum_contents(context, cksum);
-    cksum->contents = (krb5_octet *)temp;
-
-    /* success */
-cleanup:
     if (buf)
         xfree(buf);
     return code;

diff --git a/src/lib/krb5/krb/kfree.c b/src/lib/krb5/krb/kfree.c
index ab2409f1727ad6d578af69a23ab353d2a9de7b9a..6e380440506daf3ed7338f0e6af8ca606199c83f 100644 (file)
--- a/src/lib/krb5/krb/kfree.c
+++ b/src/lib/krb5/krb/kfree.c
@@ -145,6 +145,7 @@ krb5_free_checksum_contents(krb5_context context, krb5_checksum *val)
         return;
     free(val->contents);
     val->contents = NULL;
+    val->length = 0;
 }
 
 void KRB5_CALLCONV
@@ -242,10 +243,9 @@ krb5_free_data_contents(krb5_context context, krb5_data *val)
 {
     if (val == NULL)
         return;
-    if (val->data) {
-        free(val->data);
-        val->data = 0;
-    }
+    free(val->data);
+    val->data = NULL;
+    val->length = 0;
 }
 
 void KRB5_CALLCONV

diff --git a/src/tests/rdreq.c b/src/tests/rdreq.c
index c010cb2c719694b6508ad09f3d1f4972b0a9d5b7..52bec18c914768d667adc8d27e26de25d6eec577 100644 (file)
--- a/src/tests/rdreq.c
+++ b/src/tests/rdreq.c
@@ -33,6 +33,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <assert.h>
 #include <krb5.h>
 
 int
@@ -105,6 +106,7 @@ main(int argc, char **argv)
     }
 
     krb5_free_data_contents(context, &apreq);
+    assert(apreq.length == 0);
     krb5_auth_con_free(context, auth_con);
     krb5_free_creds(context, cred);
     krb5_cc_close(context, ccache);