tree bcd294877155b337ff4e7d57c7cbbff4f29dfe8e
parent 8313e13cde1df3380317a3629f1d591635293592
author David Benjamin <davidben@google.com> 1687370238 -0400
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> 1697050128 +0000

Avoid GCC's -Wdangling-pointer warning in CBB

This warning is ultimately a false positive, but does reflect an
annoyingly sharp edge in the CBB API.

Due to C's lack of destructors and |CBB|'s auto-flushing API, a failing
|CBB|-taking function may leave a dangling pointer to a child |CBB|. As
a result, the convention is callers may not write to |CBB|s that have
failed.  But, as a safety measure, we lock the |CBB| into an error
state. Once the error bit is set, |cbb->child| will not be read.

See also https://boringssl-review.googlesource.com/8840

However, there were a few codepaths in cbb.c that did not set the error
bit. Additionally, GCC does not know this invariant, so it flags a
dangling pointer warning. Fix the missing path, and explicitly null the
child pointers whenever we set the error flag.

Weirdly, the explicit null doesn't seem to be necessary, but if I inline
things manually and then delete some seemingly unrelated branches, it
becomes necessary. I assume GCC's analysis is just very fragile or
buggy, so let's just explicitly null the pointer.

This still isn't quite ideal. A |CBB| function *outside* this file may
originate an error while the |CBB| points to a local child.  In that
case we don't set the error bit and are reliant on the error convention.
Perhaps we allow |CBB_cleanup| on child |CBB|s and make every child's
|CBB_cleanup| set the error bit if unflushed. That will be convenient
for C++ callers, but very tedious for C callers. So C callers perhaps
should get a |CBB_on_error| function that can be, less tediously, stuck
in a |goto err| block. I've left this as a TODO for now. (Note the
|CBB_cleanup| strategy will capture the error bit, which is the
important one, but it cannot capture the explicit nulling. So we are
also relying on GCC not seeing through translation units right now.)

Fixed: 621
Change-Id: I9dd1c48e642fc2834940d178678f17b14009c412
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63206
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>