Detected in  issue 600808 .

+haraken@
+oilpan-reviews@

yutak@, do you have a bandwidth to look at this?

Well, wait, is PaymentItem a garbage collected type? It seems not.

It's not OK to put a non-garbage collected class into a HeapVector.

It's a valid part object (a dictionary), with a trace method; what's the issue about not putting that into a HeapVector?

Hm, yes, it actually has DISALLOW_NEW_EXCEPT_PLACEMENT_NEW. But I'm not sure
what really should happen in that case.

- Should HeapVector call the destructor of each item?
- Should HeapVectorBacking::finalize call the destructor of the elements?

Anyway this should be a problem in the Oilpan side.

> - Should HeapVector call the destructor of each item?

If the HeapVector has an inline buffer, yes. Otherwise, no.

> - Should HeapVectorBacking::finalize call the destructor of the elements?

Yes.

Note that HeapVector itself (and its inline buffer) is off-heap and HeapVectorBacking is on-heap. So it makes sense to call destructors for items in the inline buffer but not call destructors for items in HeapVectorBacking.

Does the issue happen if you run:

  PaymentDetails details;
  HeapVector<PaymentItem> vector;
  vector.append(PaymentItem());
  details.setItems(vector);
  details.setItems(HeapVector<PaymentItem>()); // <-- causes double delete???

?

> > - Should HeapVector call the destructor of each item?
>
> If the HeapVector has an inline buffer, yes. Otherwise, no.

Currectly, (Heap)Vector does not have a notion of "destruct elements if they are
in an inline buffer". Note that the backing buffer of a Vector is not
necessarily initialized (only allocated regions are constructed), so it's wrong
for HeapVectorBacking to unconditionally call the destructor for all the slots
in an out-of-line buffer.

This "destruct only in inline buffer" contract seems fragile and dangerous
to me.

Also, do elements in HeapVector get traced? Since part objects do not have
a header, I'm not sure Oilpan can deal with such objects in heap backing buffer.

Of course they're traced, as they often need to be. Lots would break if they weren't.

Not marked, only the backing store is.

> Currectly, (Heap)Vector does not have a notion of "destruct elements if they are
> in an inline buffer".

We have the logic here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/wtf/Vector.h&q=vector.h&sq=package:chromium&type=cs&l=799

> Note that the backing buffer of a Vector is not
> necessarily initialized (only allocated regions are constructed), so it's wrong
> for HeapVectorBacking to unconditionally call the destructor for all the slots
> in an out-of-line buffer.

You're right. So HeapVector supports only items against which we can unconditionally call destructors.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/heap/HeapAllocator.h&q=heapallocator&sq=package:chromium&type=cs&l=257

Specifically, HeapVector support only an item that meets either of the following conditions:

a) The item can be initialized with memset. This means that it's safe to call a destructor for a zeroed-out item (which exists at [size(), capacity())).

b) The item has a vtable. Then we can know if the item is in [size(), capacity()) or not by looking up the vtable. If the item is in [size(), capacity()), the vtable is zeroed out. Then we don't call a destructor.

Then who traces the elements in the inline buffer?

Inline buffer is traced by Vector::trace: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/wtf/Vector.h&q=vector.h&sq=package:chromium&type=cs&l=1478

In short, it's a responsibility of HeapVectorBacking to trace/finalize an out-of-line buffer. It's a responsibility of HeapVector to trace/finalize an inline buffer.

Hum, okay, then this seems like ubsan's misfire?

The failure is at:

https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/platform/heap/HeapAllocator.h#269

but it is expected behavior for this reinterpret_cast to cast an already
destructed object. It is guaranteed to be safe to do so.

The |pointer| is pointing to a not-yet-destructed out-of-line buffer. [pointer, pointer + sizeof(T)) may be uninitialized memory, but I don't think ubsan fires an error in that case. (If ubsan fires an error in that case, it means that ubsan will fire an error for all vectors that have non-0 capacity but don't yet have items.)

Vector calls destructor each time an element is removed, regardless of the
kind of the buffer (inline or out-of-line). So it's possible that a destructor
is called in some slots. This is separate from destruction in finalize().

In this case, operator= calls swap(), which calls VectorTypeOperations::swap(), which calls VectorMover::move(), which destructs the source slot after the move.

And yes, we clear the slot after the move, by calling clearUnusedSlots().

ubsan's error says:

  HeapAllocator.h:269:17: runtime error: control flow integrity check for type 'blink::PaymentItem' failed during cast to unrelated type (vtable address 0x000000000000)

which basically means we've cleared the slot.

Thanks, I'm getting a better understanding.

Why don't we hit the ubsan's error for a vector that has cleared, unused slots but don't yet have any items? (e.g., if you call reserveCapacity(3) for a vector, the vector has cleared, unused slots but don't yet have items.)

I don't know but I guess in most cases the elements fit in the inline capacity,
so we don't hit this code path frequently.

Why is there uninitialized memory, when Oilpan allocations are all zeroed on allocation?

There is no uninitialized memory. We're hitting the zeroed-out allocation and ubsan is complaining about it.

To clarify a bit: it's not UBSAN complaining, it's CFI: https://www.chromium.org/developers/testing/control-flow-integrity

Control Flow Integrity is a security mitigation technique that prevents hijacking virtual pointer. For each virtual or indirect call or a cast, it evaluates the list of possible subclasses which could be valid targets and compare the real vptr with the expected range.

Now, given that the memory is cleared out, the object is considered in the invalid state, and CFI busts the process. This is what happenned here: https://bugs.chromium.org/p/chromium/issues/detail?id=600808

runtime error: control flow integrity check for type 'blink::ShippingOption' failed during cast to unrelated type (vtable address 0x00000000
0000)
0x000000000000: note: invalid vtable

Most often I see this message when it tries to delete already destructed object, that's why double delete was my first guess. In this case, as you say, we're hitting zeroed-out allocation and try to make a reinterpret_cast for invalid object state. CFI correctly prevents this dangerous behavior.

Peter could possibly correct me, if I got the situation wrong.

krasin@'s summary of the situation is pretty much correct.

So it looks like you are casting to a polymorphic type and later determining whether the casted object has a valid vtable. That's going to fail the CFI check if the object does not in fact have a valid vtable, as (in the general case) this is exactly the sort of code that can lead to security vulnerabilities. If you intend to continue using this design, I suggest that you use a char* pointer to inspect the buffer and only cast to T* once you have validated the vtable pointer. That way, the CFI check happens only at the point where it is necessary.

i.e.

if (std::is_polymorphic<T>::value) {
        char *buffer = reinterpret_cast<char*>(pointer);
        for (unsigned i = 0; i < length; ++i) {
            char *elem = buffer + i*sizeof(T);
            if (blink::vTableInitialized(elem))
                reinterpret_cast<T*>(elem)->~T();
        }
} else {
    ...
}

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d6514453bd381827b30e0e1fcb15fb211387a51

commit 4d6514453bd381827b30e0e1fcb15fb211387a51
Author: rouslan <rouslan@chromium.org>
Date: Wed Oct 19 17:09:35 2016

Don't overwrite PaymentDetails

Overwriting shipping options in PaymentDetails causes a double-delete
due to the bug in Oilpan GC http://crbug.com/601193. A workaround until
that bug is fixed is to avoid overwriting shipping options in
PaymentDetails. However, shipping options cannot be sent to the browser
as-is. Therefore, the shipping options are overwritten in the Mojo data
structure instead.

BUG=600080, 601193

Review-Url: https://chromiumcodereview.appspot.com/2427633004
Cr-Commit-Position: refs/heads/master@{#426229}

[modify] https://crrev.com/4d6514453bd381827b30e0e1fcb15fb211387a51/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71b71dabac9102c1f2d48040ca15ca288803571f

commit 71b71dabac9102c1f2d48040ca15ca288803571f
Author: haraken <haraken@chromium.org>
Date: Thu Oct 20 22:11:08 2016

Fix CFI errors in Oilpan

Oilpan is currently casting an object to a polymorphic type to check if the object has
a vtable or not, but this causes a CFI error if the object doesn't have the polymorphic type.
This CL fixes the issue, follwing the krasin's advice on the bug.

BUG=601193

Review-Url: https://chromiumcodereview.appspot.com/2436123002
Cr-Commit-Position: refs/heads/master@{#426615}

[modify] https://crrev.com/71b71dabac9102c1f2d48040ca15ca288803571f/third_party/WebKit/Source/platform/heap/HeapAllocator.h
[modify] https://crrev.com/71b71dabac9102c1f2d48040ca15ca288803571f/third_party/WebKit/Source/platform/heap/TraceTraits.h