Placement new seems to confuse CFI |
||
Issue descriptionThe latest BoringSSL roll got reverted due to CFI flagging things. https://chromium-review.googlesource.com/c/593107/ Playing around with CFI locally, this looks like a CFI bug. Due to some goofy (hopefully eventually resolvable) constraints by one of our consumers, while we can finally use C++, we cannot depend the runtime and call new/delete, so we use a "custom" allocator with malloc/free and all the other nastiness. The attached file is a small example which trips CFI. I found issue #713293 which looks like it might be the same thing? Does CFI just not handle this sort of thing right now? In that case, it sounds like we should add *third_party/boringssl/* to the blacklist? Does that seem reasonable? BoringSSL's current constraints are such that every vtabled cast will trip this.
,
Jul 31 2017
Ohhh... that works! Thanks! Closing.
,
Jul 31 2017
The following revision refers to this bug: https://boringssl.googlesource.com/boringssl/+/a4cb62f0ae6566068aef0742eb1cd46227d7dffd commit a4cb62f0ae6566068aef0742eb1cd46227d7dffd Author: David Benjamin <davidben@google.com> Date: Mon Jul 31 18:04:44 2017 Fix build against LLVM CFI. The first line of bssl::New is invalid in LLVM CFI as we are casting a pointer to T before the object is constructed. Instead, we should leave it as void* and only use it as a T* afterward being constructed. Bug: chromium:750445 Change-Id: I0ae60c2a7e541b45bc0155dd8f359b662f561dcc Reviewed-on: https://boringssl-review.googlesource.com/18684 Commit-Queue: David Benjamin <davidben@google.com> Commit-Queue: Steven Valdez <svaldez@google.com> Reviewed-by: Steven Valdez <svaldez@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> [modify] https://crrev.com/a4cb62f0ae6566068aef0742eb1cd46227d7dffd/ssl/internal.h |
||
►
Sign in to add a comment |
||
Comment 1 by p...@chromium.org
, Jul 31 2017This behaviour is expected, and is being caused by the reinterpret_cast to the object type that this code is doing between allocation and construction with the placement new, as CFI will forbid a reinterpret_cast to a dynamic type if the object is not yet constructed. You should be able to fix this by moving the reinterpret_cast to after construction: diff --git a/ssl/internal.h b/ssl/internal.h index c8a96a43..9a7f3ba5 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -181,13 +181,13 @@ struct SSL_HANDSHAKE; * Note: unlike |new|, this does not support non-public constructors. */ template <typename T, typename... Args> T *New(Args &&... args) { - T *t = reinterpret_cast<T *>(OPENSSL_malloc(sizeof(T))); + void *t = OPENSSL_malloc(sizeof(T)); if (t == nullptr) { OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); return nullptr; } new (t) T(std::forward<Args>(args)...); - return t; + return reinterpret_cast<T *>(t); } /* Delete behaves like |delete| but uses |OPENSSL_free| to release memory.