A tricky bug, about memset and raw_ptr.

[Arthur Sonzogni]

On Wed, Nov 23, 2022 at 3:39 PM Gabriel Charette <g...@google.com> wrote:

Wow, that's a tricky one! Thanks to everyone involved here, from identifying the large regression to diagnosis :).  

I suggest sending this doc to c...@chromium.org and, separately, to chrome-securit...@google.com

On Gabriel suggestion: +c...@chromium.org 

See the doc

This is about a huge memory spike on Linux when BackupRefPtr was enabled. It is an interesting bug, because it was quite difficult to find the root cause.

The cause was an incorrect use of `memset` over an object containing non-trivially copyable members, in this case, a `raw_ptr`. After zeroing its memory, it was not able to decrement the refcount. It caused partition_alloc quarantine to grow forever.

Fortunately, @Daniel Cheng suggested adding a new clang-tidy warning `bugprone-undefined-memory-manipulation`, to prevent this class of bug from happening again. (bug)

Arthur @arthursonzogni

[Joe Mason]

I see on the bug that there was an earlier fix targeting the same memory increase: https://crrev.com/c/4054426

Make sure to std::move `event.window_` in the move constructors to
properly release the raw_ptr being moved and to avoid quarantine bloat.

Did you need to fix this AND remove memset to avoid the quarantine bloat, or was just one of them enough?

The diff was

Event::Event(Event&& event)
    : send_event_(event.send_event_),
...

<      window_(event.window_) {

>      window_(std::move(event.window_)) {

If I understand right, the original version of that copied the `window_` raw_ptr, which would increase the refcount. Then after the move, the original `event` is in an undefined state, so when it goes out of scope its destructor (and its members destructors don't run), so it never release's its ref to the raw_ptr. Did I get that right?

Would a shared_ptr or scoped_refptr have the same error if someone accidentally copied them instead of moving in a move constructor? I find that surprisingly fragile.

Thanks,

Joe

--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAzos5GsfLroJXg2mcC%3DDZT6dL%2BznSNQ_RLyEmJ-UVQM0Wp7ZQ%40mail.gmail.com.

[Joe Mason]

Dana pointed out in chat that I got semantics of move assignment wrong. After the move constructor runs, the original `event` isn't left in a fully undefined state, it's required to be in a state that can be destroyed, and its destructor WILL run when it goes out of scope. So a shared_ptr that's copied instead of moved will just end up with 2 references until the original `event` goes out of scope.

Which changes my question: why didn't that happen with the `raw_ptr`? Is this a potential bug in BackupRefPtr?

[Daniel Cheng]

I think if *just* the move had been added, it /might/ have been enough. But memset() is arguably wrong here too.

The reason it was leaking was because:

- the ref was copied not moved

- the memset() overwrote the raw_ptr's value without actually decrementing the refcount

Daniel

[Daniel Cheng]

Oh I misread that both fixes were in one CL. I might not know what I'm talking about if that wasn't the case.

Daniel

[Arthur Sonzogni]

I think if *just* the move had been added, it /might/ have been enough. But memset() is arguably wrong here too.

The reason it was leaking was because:

- the ref was copied not moved

- the memset() overwrote the raw_ptr's value without actually decrementing the refcount

Exactly.

The reviewer initially declined removing the memset, and the fix landed without it. I insisted and merged follow-up.

This created some confusion about which patch fixed the bug.

 

[Gabriel Charette]

I agree that the memset was the problem (as it evades the raw_ptr's destructor). The std::move was just an optimization, not the bug fix, as a copy would merely temporarily result in two refs (both owned by their copies).