yggdrasil/ring
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Handle CBB_cleanup on child CBBs more gracefully.

Browse Source 
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>
This commit is contained in:
David Benjamin 2019-04-09 20:17:30 -05:00 committed by Adam Langley
parent be7006adac
commit ad9eee1628
2 changed files with 13 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
crypto/bytestring/cbb.c
View File

@ -44,7 +44,7 @@ static int cbb_init(CBB *cbb, uint8_t *buf, size_t cap) {
base->error = 0;
cbb->base = base;
cbb->is_top_level = 1;
cbb->is_child = 0;
return 1;
}
@ -76,11 +76,14 @@ int CBB_init_fixed(CBB *cbb, uint8_t *buf, size_t len) {
}
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 @@ static int cbb_buffer_add_u(struct cbb_buffer_st *base, uint64_t v,
}
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 @@ static int cbb_add_length_prefixed(CBB *cbb, CBB *out_contents,
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 @@ int CBB_add_asn1(CBB *cbb, CBB *out_contents, unsigned tag) {
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;

4
include/openssl/bytestring.h
View File

@ -345,9 +345,9 @@ struct cbb_st {
// 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