Help needed fixing invalid C++

[Avi Drissman]

I was tree sheriff last Friday, and some broken bots led Victor and me to a very interesting part of Chromium's code.

If you take a look at cc's ListContainer::AppendByMoving, you'll find:

    memcpy(new_item, static_cast<void*>(item), max_size_for_derived_class);

That is... possibly OK if the type is trivially-copyable. So as an experiment I put in a static assert to verify it was so. And hundreds of lines of the unit test then failed. So I commented out each unit test that failed to compile because of the new requirement for trivial copyability. And it compiled fine.

So on one hand, we don't seem to be shipping any code that relies on undefined C++. On the other hand, the unit tests for that code are full of code that exercises the code that we do ship in a way that makes it clear that the intention is to handle non-trivially-copyable types.

Can one of you, C++ peeps, take a look at the code? Is there a fix that you can do to the ListContainer to make it able to fulfill the requirements of the unit tests and the other expectations of the code without having it be undefined?

Thanks!

Avi

[Avi Drissman]

Digging a bit, the only use of ListContainer is in QuadList, which inherits from ListContainer<DrawQuad>. DrawQuad has a virtual destructor and a pure virtual method, which AFAIK makes it non-trivially-copyable, so I might be mistaken as to the definedness of our shipping code.

Avi

[Chris Blume]

I think I agree with you.
Do we never have lists of mixed derived types?
Or rather, do we only call ListContainer::AppendByMoving() on memory that previously held DerivedElementType?
A part of me think this could be magically working thanks to de-virtualization?


I only looked at this code for a little while but I'm not sure I understand why it doesn't just take an r-value reference.
That seems to preserve the intention of the function while allowing a safe construction.

Chris Blume | Software Engineer | cbl...@google.com | +1-614-929-9221

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAaRiQU8Bpa5JB2ZTxK6TXw_RY2Vkkh2aTKD9TpzX%3D5oSg%40mail.gmail.com.

[dan...@chromium.org]

I dont see any uses of this method in production code anymore. I agree it could be rvalue references now, but it could just be removed instead it seems.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAPHsw37gAe7WykV2EokPKByEy5sJmjmRcgKeNa7bc9SWw7aLyg%40mail.gmail.com.

[Avi Drissman]

In an attempt to double-check your assertion that ListContainer::AppendByMoving wasn't actually used anywhere, I did a search for "AppendByMoving" and found ContiguousContainer::AppendByMoving which does something very similar. It also has just one non-test use as the parent of DisplayItemList, which is a ContiguousContainer of DisplayItem which is also not trivially-copyable. And DisplayItemList::AppendByMoving definitely calls ContiguousContainer::AppendByMoving.

(As a side note, there exists "DISABLE_CFI_PERF" which is available to disable CFI for performance reasons. I think it needs an audit as it's used in ContiguousContainer::Clear to avoid the CFI bot failing on this invalid code and I imagine it's used elsewhere improperly.)

[dan...@chromium.org]

On Mon, Apr 16, 2018 at 12:21 PM, Avi Drissman <a...@chromium.org> wrote:

In an attempt to double-check your assertion that ListContainer::AppendByMoving wasn't actually used anywhere, I did a search for "AppendByMoving" and found ContiguousContainer::AppendByMoving which does something very similar. It also has just one non-test use as the parent of DisplayItemList, which is a ContiguousContainer of DisplayItem which is also not trivially-copyable. And DisplayItemList::AppendByMoving definitely calls ContiguousContainer::AppendByMoving.

Thanks, you're right. It also looks like it should std::move() and call operator=(T&&), where the operator sets is_tombstone_ = true on the T&&, instead of the memcpy-default-constructor dance.

[Avi Drissman]

https://crbug.com/833475 filed.

If there's any C++ expert who wants to clean this up, feel free to take it.