From 5736d76ae60c1746cf6fe60466df655c91bf7a7b Mon Sep 17 00:00:00 2001 From: Martijn Vels Date: Wed, 7 Dec 2022 07:14:42 -0800 Subject: Remove possible UB from CopyRaw() This change removes the 'create by single memcpy' CopyRaw() logic to avoid potential UB from the atomic refcount data being included in the memcpy. While we know this works on all supported platforms, it is officially UB, and we should change it to something else. This change changes the CopyRaw() method to explicitly create a (default initialized) instance, initialize `length` and `refcount`, and then memcpy the remaining contents which are trivial uint8_t and CordRep* values. PiperOrigin-RevId: 493596072 Change-Id: I46618883eb1c7c9ed9eb083f4d3e7fc501f23df5 --- absl/strings/internal/cord_rep_btree.h | 35 ++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) (limited to 'absl/strings/internal/cord_rep_btree.h') diff --git a/absl/strings/internal/cord_rep_btree.h b/absl/strings/internal/cord_rep_btree.h index eed5609e..4209e512 100644 --- a/absl/strings/internal/cord_rep_btree.h +++ b/absl/strings/internal/cord_rep_btree.h @@ -446,9 +446,9 @@ class CordRepBtree : public CordRep { template static CordRepBtree* NewLeaf(absl::string_view data, size_t extra); - // Creates a raw copy of this Btree node, copying all properties, but - // without adding any references to existing edges. - CordRepBtree* CopyRaw() const; + // Creates a raw copy of this Btree node with the specified length, copying + // all properties, but without adding any references to existing edges. + CordRepBtree* CopyRaw(size_t new_length) const; // Creates a full copy of this Btree node, adding a reference on all edges. CordRepBtree* Copy() const; @@ -666,15 +666,28 @@ inline void CordRepBtree::Unref(absl::Span edges) { } } -inline CordRepBtree* CordRepBtree::CopyRaw() const { - auto* tree = static_cast(::operator new(sizeof(CordRepBtree))); - memcpy(static_cast(tree), this, sizeof(CordRepBtree)); - new (&tree->refcount) RefcountAndFlags; +inline CordRepBtree* CordRepBtree::CopyRaw(size_t new_length) const { + CordRepBtree* tree = new CordRepBtree; + + // `length` and `refcount` are the first members of `CordRepBtree`. + // We initialize `length` using the given length, have `refcount` be set to + // ref = 1 through its default constructor, and copy all data beyond + // 'refcount' which starts with `tag` using a single memcpy: all contents + // except `refcount` is trivially copyable, and the compiler does not + // efficiently coalesce member-wise copy of these members. + // See https://gcc.godbolt.org/z/qY8zsca6z + // # LINT.IfChange(copy_raw) + tree->length = new_length; + uint8_t* dst = &tree->tag; + const uint8_t* src = &tag; + const ptrdiff_t offset = src - reinterpret_cast(this); + memcpy(dst, src, sizeof(CordRepBtree) - static_cast(offset)); return tree; + // # LINT.ThenChange() } inline CordRepBtree* CordRepBtree::Copy() const { - CordRepBtree* tree = CopyRaw(); + CordRepBtree* tree = CopyRaw(length); for (CordRep* rep : Edges()) CordRep::Ref(rep); return tree; } @@ -683,8 +696,7 @@ inline CordRepBtree* CordRepBtree::CopyToEndFrom(size_t begin, size_t new_length) const { assert(begin >= this->begin()); assert(begin <= this->end()); - CordRepBtree* tree = CopyRaw(); - tree->length = new_length; + CordRepBtree* tree = CopyRaw(new_length); tree->set_begin(begin); for (CordRep* edge : tree->Edges()) CordRep::Ref(edge); return tree; @@ -694,8 +706,7 @@ inline CordRepBtree* CordRepBtree::CopyBeginTo(size_t end, size_t new_length) const { assert(end <= capacity()); assert(end >= this->begin()); - CordRepBtree* tree = CopyRaw(); - tree->length = new_length; + CordRepBtree* tree = CopyRaw(new_length); tree->set_end(end); for (CordRep* edge : tree->Edges()) CordRep::Ref(edge); return tree; -- cgit v1.2.3