CFI prevents std::vectors of virtual classes |
||
Issue description
This came up during a recent CL [1]. When emplacing into the middle of a std::vector<T> with sufficient capacity libc++ constructs a temporary object in unintialized memory [2, 3]. If T has virtual methods, this triggers a CFI error, as the vtable is invalid at that point.
Minimal Reproduction Case:
struct Foo {
virtual ~Foo() = default;
};
std::vector<Foo> foos = { Foo() };
foos.reserve(2);
foos.emplace(foos.begin(), Foo()); // boom
I'm not sure myself if this is a bug or a feature, as having virtual methods in value type classes should be rare, but I would like to hear your opinion on this.
[1] https://crrev.com/c/1161022
[2] https://github.com/llvm-mirror/libcxx/blob/e25adff05a0c53f92f0ca353a97f14ac90111a88/include/vector#L1861
[3] https://github.com/llvm-mirror/libcxx/blob/e25adff05a0c53f92f0ca353a97f14ac90111a88/include/memory#L5635
,
Aug 3
Yep, this is the crash:
_Tp *__addr() { return reinterpret_cast<_Tp *>(addressof(__v)); }
Regarding std::get_temporary_buffer: It seems like the Android crashes of my CL happen because of it [1]. Would it be possible to annotate the Android version for CFI as well?
[1] https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8939289062935579600/+/steps/device_unittests_on_Android_device_Nexus_5X/0/logs/BluetoothTest.AdvertisementData_ConnectionDuringDiscovery/0
,
Aug 3
The upstream annotation in get_temporary_buffer is from Jan 2018: https://reviews.llvm.org/D42146 There are also blacklist entries for it, which do not match the ndk1:: prefix, unfortunately.
,
Aug 3
,
Aug 3
We'll need to add the _LIBCPP_NO_CFI annotation upstream and cherry pick it as well as the get_temporary_buffer change into Chromium's copy of the NDK. The blacklist entries for libc++ should be removed because the proper place for these annotations is in the source code.
,
Aug 14
,
Aug 15
Landed upstream in r339797 and sent https://chromium-review.googlesource.com/c/android_ndk/+/1176190 with the cherry-pick.
,
Aug 15
The following revision refers to this bug: https://chromium.googlesource.com/android_ndk/+/4e2cea441bfd43f0863d14f57b1e1844260b9884 commit 4e2cea441bfd43f0863d14f57b1e1844260b9884 Author: Peter Collingbourne <pcc@google.com> Date: Wed Aug 15 20:07:25 2018 Cherry-pick r322744 and r339797 from upstream LLVM libcxx. This adds a couple of _LIBCPP_NO_CFI annotations that we need to silence CFI bad cast errors. Bug: 870677 Change-Id: Ifabee936c021faf568dd45299cde052db9c739af Reviewed-on: https://chromium-review.googlesource.com/1176190 Tested-by: Peter Collingbourne <pcc@chromium.org> Reviewed-by: agrieve <agrieve@chromium.org> [modify] https://crrev.com/4e2cea441bfd43f0863d14f57b1e1844260b9884/sources/cxx-stl/llvm-libc++/include/memory [modify] https://crrev.com/4e2cea441bfd43f0863d14f57b1e1844260b9884/README.chromium
,
Aug 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/69af5245926d933824cb3a9b0a2df4816270c55f commit 69af5245926d933824cb3a9b0a2df4816270c55f Author: Peter Collingbourne <pcc@chromium.org> Date: Thu Aug 16 03:30:15 2018 Roll third_party/android_ndk. This rolls in a change to cherry-pick r322744 and r339797 from upstream LLVM libcxx. Bug: 870677 Change-Id: I4350775457b5defb31f7f222e589482b5378ee4f Reviewed-on: https://chromium-review.googlesource.com/1176345 Reviewed-by: agrieve <agrieve@chromium.org> Commit-Queue: Peter Collingbourne <pcc@chromium.org> Cr-Commit-Position: refs/heads/master@{#583524} [modify] https://crrev.com/69af5245926d933824cb3a9b0a2df4816270c55f/DEPS |
||
►
Sign in to add a comment |
||
Comment 1 by euge...@chromium.org
, Aug 3