CFI base-to-derived cast error in SentinelLinkedList.h

[Christopher Reid]

We're trying to get CFI running with JSC and one of the errors we're seeing is:
`SentinelLinkedList.h:63:24: runtime error: control flow integrity check for type 'JSC::Watchpoint' failed during base-to-derived cast (vtable address 0x000000000000)`

(lldb) bt
* thread #6, name = ' via C++ thread', stop reason = Cfi bad type
    frame #0: 0x0000000000877810 testapi`__ubsan_on_report at ubsan_monitor.cc:39:40
    frame #1: 0x0000000000871d54 testapi`::~Diag() at ubsan_diag.cc:354:29
    frame #2: 0x00000000008789b4 testapi`::__ubsan_handle_cfi_bad_type() at ubsan_handlers_cxx.cc:133:3
    frame #3: 0x00000000008771b2 testapi`__ubsan_handle_cfi_check_fail at ubsan_handlers.cc:846:5
  * frame #4: 0x00000000009339a1 testapi`WTF::BasicRawSentinelNode<JSC::Watchpoint>::next(this=0x00007ffff0df93a0) at SentinelLinkedList.h:63:24
    frame #5: 0x0000000000a34bc5 testapi`WTF::SentinelLinkedList<JSC::Watchpoint, WTF::BasicRawSentinelNode<JSC::Watchpoint> >::begin(this=0x00007ffff0df93a0) at SentinelLinkedList.h:151:43
    frame #6: 0x0000000000a34b8b testapi`WTF::SentinelLinkedList<JSC::Watchpoint, WTF::BasicRawSentinelNode<JSC::Watchpoint> >::isEmpty(this=0x00007ffff0df93a0) at SentinelLinkedList.h:102:29
    frame #7: 0x0000000000a34b3d testapi`JSC::WatchpointSet::~WatchpointSet(this=0x00007ffff0df9398) at Watchpoint.cpp:70:19
    ...

This looks like a valid base-to-derived error because the SentinelLinkedList head and tail nodes are constructed as the base class BasicRawSentinelNode<JSC::Watchpoint>. m_headSentinel.next() tries to cast the object constructed as BasicRawSentinelNode<JSC::Watchpoint>* to a JSC::Watchpoint*. Those two objects are also different sizes. It doesn't seem like the head and tail nodes are dereferenced after a prev/next call, but it will probably be unsafe if they do end up being dereferenced.

[Filip Pizlo]

This looks like it’s not a bug at all.

[Yusuke Suzuki]

I think this use case exists everywhere in WebKit, and introducing some annotation / blacklisting this is not worth doing for this case.
The best way here would be just disabling this check in CFI side.

[Michael Catanzaro]

How is this OK? Why would you do this intentionally?

[Filip Pizlo]

(In reply to Michael Catanzaro from comment #3)
> How is this OK? Why would you do this intentionally?

Because it's valid C.  We're only accessing fields that exist.  We're just using the wrong enclosing type to do it.