commit a33915d69045dd17ce50749e011dfead2fa969ab
author Adam Langley <agl@google.com> Mon Nov 16 12:04:32 2015 -0800
committer Adam Langley <agl@google.com> Mon Nov 16 21:59:46 2015 +0000
tree 1ae7e050660f7d252f049140e108c7d750b5788f
parent c5c85defb2f3bfb6a462a47da8e2e2c354dbf926

Have |CBB_init| zero the |CBB| before any possible failures.

People expect to do:

CBB foo;

if (!CBB_init(&foo, 100) ||
    …
    …) {
  CBB_cleanup(&foo);
  return 0;
}

However, currently, if the allocation of |initial_capacity| fails in
|CBB_init| then |CBB_cleanup| will operate on uninitialised values. This
change makes the above pattern safe.

Change-Id: I3e002fda8f0a3ac18650b504e7e84a842d4165ca
Reviewed-on: https://boringssl-review.googlesource.com/6495
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>

crypto/bytestring/bytestring_test.cc

diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index f25c186..4c2add6 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -316,6 +316,22 @@
   return true;
 }
 
+static bool TestCBBAllocFailure() {
+  CBB cbb;
+  memset(&cbb, 0x42, sizeof(cbb));
+
+  if (CBB_init(&cbb, (size_t)-1)) {
+    fprintf(stderr, "Excessive allocation successful!\n");
+    CBB_cleanup(&cbb);
+    return false;
+  }
+
+  /* |CBB_init| should have cleared |cbb| before failing therefore this should
+   * not crash. */
+  CBB_cleanup(&cbb);
+  return true;
+}
+
 static bool TestCBBFinishChild() {
   CBB cbb, child;
   uint8_t *out_buf;
@@ -716,6 +732,7 @@
       !TestGetASN1() ||
       !TestCBBBasic() ||
       !TestCBBFixed() ||
+      !TestCBBAllocFailure() ||
       !TestCBBFinishChild() ||
       !TestCBBMisuse() ||
       !TestCBBPrefixed() ||

crypto/bytestring/cbb.c

diff --git a/crypto/bytestring/cbb.c b/crypto/bytestring/cbb.c
index 434ec13..74573c9 100644
--- a/crypto/bytestring/cbb.c
+++ b/crypto/bytestring/cbb.c
@@ -25,6 +25,7 @@
 }
 
 static int cbb_init(CBB *cbb, uint8_t *buf, size_t cap) {
+  /* This assumes that |cbb| has already been zeroed. */
   struct cbb_buffer_st *base;
 
   base = OPENSSL_malloc(sizeof(struct cbb_buffer_st));
@@ -37,16 +38,15 @@
   base->cap = cap;
   base->can_resize = 1;
 
-  memset(cbb, 0, sizeof(CBB));
   cbb->base = base;
   cbb->is_top_level = 1;
   return 1;
 }
 
 int CBB_init(CBB *cbb, size_t initial_capacity) {
-  uint8_t *buf;
+  CBB_zero(cbb);
 
-  buf = OPENSSL_malloc(initial_capacity);
+  uint8_t *buf = OPENSSL_malloc(initial_capacity);
   if (initial_capacity > 0 && buf == NULL) {
     return 0;
   }
@@ -60,6 +60,8 @@
 }
 
 int CBB_init_fixed(CBB *cbb, uint8_t *buf, size_t len) {
+  CBB_zero(cbb);
+
   if (!cbb_init(cbb, buf, len)) {
     return 0;
   }