| commit | 5555991d3729d4231671214f0b9ba4858a5a8a81 | [log] [tgz] |
|---|---|---|
| author | David Benjamin <davidben@google.com> | Wed Jun 21 13:57:18 2023 -0400 |
| committer | Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> | Wed Oct 11 18:48:48 2023 +0000 |
| tree | bcd294877155b337ff4e7d57c7cbbff4f29dfe8e | |
| parent | 8313e13cde1df3380317a3629f1d591635293592 [diff] |
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>
BoringSSL is a fork of OpenSSL that is designed to meet Google's needs.
Although BoringSSL is an open source project, it is not intended for general use, as OpenSSL is. We don't recommend that third parties depend upon it. Doing so is likely to be frustrating because there are no guarantees of API or ABI stability.
Programs ship their own copies of BoringSSL when they use it and we update everything as needed when deciding to make API changes. This allows us to mostly avoid compromises in the name of compatibility. It works for us, but it may not work for you.
BoringSSL arose because Google used OpenSSL for many years in various ways and, over time, built up a large number of patches that were maintained while tracking upstream OpenSSL. As Google's product portfolio became more complex, more copies of OpenSSL sprung up and the effort involved in maintaining all these patches in multiple places was growing steadily.
Currently BoringSSL is the SSL library in Chrome/Chromium, Android (but it's not part of the NDK) and a number of other apps/programs.
Project links:
There are other files in this directory which might be helpful: