Handle CBB_cleanup on child CBBs more gracefully.

Child and root CBBs share a type, but are different kinds of things. C++
programmers sometimes mistakenly believe they should use ScopedCBB for
everything. This mostly works because we NULL cbb->child->base on flush,
making CBB_cleanup a no-op. This zeroing also skips the assert in
CBB_cleanup. (If we ran it unconditionally, CBB_zero + CBB_cleanup would
not work.)

However, if a CBB operation fails and a function returns early, the
child CBB is not cleared. ScopedCBB will then call CBB_cleanup which
trips the assert but, in release build, misbehaves.

Run the assert unconditionally and, when the assert fails, still behave
well. To make this work with CBB_zero, negate is_top_level to is_child,
so a flushed child CBB and a (presumably) root CBB in the zero state are
distinguishable.

Update-Note: Code that was using CBB wrong may trip an assert in debug builds.
Change-Id: Ifea7759e1d0331f2e727c59bbafa355d70fb9dba
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/35524
Reviewed-by: Adam Langley <agl@google.com>

crypto/bytestring/cbb.c

diff --git a/crypto/bytestring/cbb.c b/crypto/bytestring/cbb.c
index 7998a48..1ddc73c 100644
--- a/crypto/bytestring/cbb.c
+++ b/crypto/bytestring/cbb.c
@@ -44,7 +44,7 @@
   base->error = 0;
 
   cbb->base = base;
-  cbb->is_top_level = 1;
+  cbb->is_child = 0;
   return 1;
 }
 
@@ -76,11 +76,14 @@
 }
 
 void CBB_cleanup(CBB *cbb) {
-  if (cbb->base) {
-    // Only top-level |CBB|s are cleaned up. Child |CBB|s are non-owning. They
-    // are implicitly discarded when the parent is flushed or cleaned up.
-    assert(cbb->is_top_level);
+  // Child |CBB|s are non-owning. They are implicitly discarded and should not
+  // be used with |CBB_cleanup| or |ScopedCBB|.
+  assert(!cbb->is_child);
+  if (cbb->is_child) {
+    return;
+  }
 
+  if (cbb->base) {
     if (cbb->base->can_resize) {
       OPENSSL_free(cbb->base->buf);
     }
@@ -169,7 +172,7 @@
 }
 
 int CBB_finish(CBB *cbb, uint8_t **out_data, size_t *out_len) {
-  if (!cbb->is_top_level) {
+  if (cbb->is_child) {
     return 0;
   }
 
@@ -310,6 +313,7 @@
   OPENSSL_memset(prefix_bytes, 0, len_len);
   OPENSSL_memset(out_contents, 0, sizeof(CBB));
   out_contents->base = cbb->base;
+  out_contents->is_child = 1;
   cbb->child = out_contents;
   cbb->child->offset = offset;
   cbb->child->pending_len_len = len_len;
@@ -381,6 +385,7 @@
 
   OPENSSL_memset(out_contents, 0, sizeof(CBB));
   out_contents->base = cbb->base;
+  out_contents->is_child = 1;
   cbb->child = out_contents;
   cbb->child->offset = offset;
   cbb->child->pending_len_len = 1;

include/openssl/bytestring.h

diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h
index 75b8434..029c2be 100644
--- a/include/openssl/bytestring.h
+++ b/include/openssl/bytestring.h
@@ -345,9 +345,9 @@
   // length-prefix, or zero if no length-prefix is pending.
   uint8_t pending_len_len;
   char pending_is_asn1;
-  // is_top_level is true iff this is a top-level |CBB| (as opposed to a child
+  // is_child is true iff this is a child |CBB| (as opposed to a top-level
   // |CBB|). Top-level objects are valid arguments for |CBB_finish|.
-  char is_top_level;
+  char is_child;
 };
 
 // CBB_zero sets an uninitialised |cbb| to the zero state. It must be