FlatAllocator code causes cfi-unrelated-cast Clang CFI issue

[tommynyquist]

What version of protobuf and what language are you using? Version: 3.20.0 Language: C++

What operating system (Linux, Windows, ...) and version? Linux

What runtime / compiler are you using (e.g., python version or gcc version) clang++

clang version 15.0.0 (https://github.com/llvm/llvm-project/ 4dcb42fae57238736105d43be54ad380f9fbf398)
Target: x86_64-unknown-linux-gnu
Thread model: posix

What did you do? Steps to reproduce the behavior:

1. On a Linux distro that supports building Chromium, check out Chromium following these steps: https://chromium.googlesource.com/chromium/src/+/main/docs/linux/build_instructions.md
2. Apply this patch: https://chromium-review.googlesource.com/c/chromium/src/+/3594212 , e.g. git fetch https://chromium.googlesource.com/chromium/src refs/changes/12/3594212/24 && git cherry-pick FETCH_HEAD
3. Execute gn gen out/Default --args='dcheck_always_on=true is_cfi=true is_component_build=false is_debug=false use_cfi_cast=true use_cfi_diag=true use_cfi_icall=true use_thin_lto=true'
4. Execute ninja -C out/Default chrome/test:profile_proto_db_test_proto_gen

What did you expect to see A successful build.

What did you see instead? A Clang CFI issue. Example Chromium builder failure:

FAILED: gen/chrome/browser/persisted_state_db/profile_proto_db_test_proto.pb.h gen/chrome/browser/persisted_state_db/profile_proto_db_test_proto.pb.cc pyproto/chrome/browser/persisted_state_db/profile_proto_db_test_proto_pb2.py 
python3 ../../tools/protoc_wrapper/protoc_wrapper.py profile_proto_db_test_proto.proto --protoc ./protoc --proto-in-dir ../../chrome/browser/persisted_state_db --cc-out-dir gen/chrome/browser/persisted_state_db --py-out-dir pyproto/chrome/browser/persisted_state_db
../../third_party/protobuf/src/google/protobuf/descriptor.cc:319:12: runtime error: control flow integrity check for type 'google::protobuf::SourceCodeInfo' failed during cast to unrelated type (vtable address 0x561c2cae20a0)
0x561c2cae20a0: note: invalid vtable
 00 00 00 00  f0 16 ae 2c 1c 56 00 00  00 00 00 00 00 00 00 00  20 f6 ad 2c 1c 56 00 00  00 00 00 00
              ^ 
../../third_party/protobuf/src/google/protobuf/descriptor.cc:319:12: note: check failed in /b/s/w/ir/cache/builder/src/out/Release/protoc, vtable located in [heap]
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../third_party/protobuf/src/google/protobuf/descriptor.cc:319:12 in 
Protoc has returned non-zero status: 1

Anything else we should know about your project / environment N/A

Notes The following current workaround for cfi-unrelated-cast does not work for any classes with vtables:

  // Avoid the reinterpret_cast if the array is empty.
  // Clang's Control Flow Integrity does not like the cast pointing to memory
  // that is not yet initialized to be of that type.
  // (from -fsanitize=cfi-unrelated-cast)
  template <typename U>
  U* Begin() const {
    int begin = BeginOffset<U>(), end = EndOffset<U>();
    if (begin == end) return nullptr;
    return reinterpret_cast<U*>(data() + begin);
  }

  template <typename U>
  U* End() const {
    int begin = BeginOffset<U>(), end = EndOffset<U>();
    if (begin == end) return nullptr;
    return reinterpret_cast<U*>(data() + end);
  }

As part of the roll to protobuf 3.20 in Chromium, we are temporarily changing that code (diff from initial patch set) to:

  template <typename U>
  __attribute__((no_sanitize("cfi-unrelated-cast", "vptr")))
  U* Begin() const {
    return reinterpret_cast<U*>(data() + BeginOffset<U>());
  }

  template <typename U>
  __attribute__((no_sanitize("cfi-unrelated-cast", "vptr")))
  U* End() const {
    return reinterpret_cast<U*>(data() + EndOffset<U>());
  }

This only ignores the error though.

[pkasting]

Wonder if this is a case for std::launder()?

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

[pkasting]

This is still valid; can it be reopened?

[googleberg]

Yes we can reopen this. @sbenzaquen can you take a look at this?

[sbenzaquen]

I think the problem here is with End(), as it points past-the-end and not to a real U. Begin() should always return a pointer to a valid object.

I think a solution is to calculate end via addition from a valid pointer instead. Eg

   template <typename U>
   U* End() const {
-    int begin = BeginOffset<U>(), end = EndOffset<U>();
-    if (begin == end) return nullptr;
-    return reinterpret_cast<U*>(data() + end);
+    return Begin<U>() + (EndOffset<U>() - BeginOffset<U>()) / sizeof(U);
   }

It might be faster if someone that can easily reproduce this issue can test the patch.