diff options
author | 2022-12-07 07:14:42 -0800 | |
---|---|---|
committer | 2022-12-07 07:15:19 -0800 | |
commit | 5736d76ae60c1746cf6fe60466df655c91bf7a7b (patch) | |
tree | f2a307cbb31cc2de71ba424784626d3b3a2568f8 /absl/strings/internal/cord_rep_btree.h | |
parent | c96db73c09dbb528eca6d19a50bd258b37e9fd5e (diff) |
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
Diffstat (limited to 'absl/strings/internal/cord_rep_btree.h')
-rw-r--r-- | absl/strings/internal/cord_rep_btree.h | 35 |
1 files changed, 23 insertions, 12 deletions
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 <EdgeType edge_type> 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<CordRep* const> edges) { } } -inline CordRepBtree* CordRepBtree::CopyRaw() const { - auto* tree = static_cast<CordRepBtree*>(::operator new(sizeof(CordRepBtree))); - memcpy(static_cast<void*>(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<const uint8_t*>(this); + memcpy(dst, src, sizeof(CordRepBtree) - static_cast<size_t>(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; |