[PATCH] D92727: [CodeGen][MSan] Don't use offsets of zero-sized fields

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka created this revision.
vitalybuka added reviewers: eugenis, morehouse.
vitalybuka requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Such fields will likely have offset zero making
__sanitizer_dtor_callback poisoning wrong regions.
E.g. it can poison base class member from derived class constructor.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92727

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGenCXX/sanitize-dtor-zero-size-field.cpp


Index: clang/test/CodeGenCXX/sanitize-dtor-zero-size-field.cpp
===================================================================
--- clang/test/CodeGenCXX/sanitize-dtor-zero-size-field.cpp
+++ clang/test/CodeGenCXX/sanitize-dtor-zero-size-field.cpp
@@ -1,8 +1,6 @@
 // RUN: %clang_cc1 -O0 -fsanitize=memory -fsanitize-memory-use-after-dtor -disable-llvm-passes -std=c++20 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s --implicit-check-not "call void @__sanitizer_dtor_callback"
 // RUN: %clang_cc1 -O1 -fsanitize=memory -fsanitize-memory-use-after-dtor -disable-llvm-passes -std=c++20 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s --implicit-check-not "call void @__sanitizer_dtor_callback"
 
-// This test convers invalid behaviour which will be fixed in followup patches.
-
 struct Empty {};
 
 struct EmptyNonTrivial {
@@ -115,7 +113,7 @@
 static_assert(sizeof(Struct) == 24);
 } // namespace T6
 // CHECK-LABEL: define {{.*}} @_ZN5empty2T66StructD2Ev(
-// CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 21)
+// CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 13)
 // CHECK-NEXT:    ret void
 
 namespace T7 {
@@ -130,7 +128,7 @@
 } // namespace T7
 // CHECK-LABEL: define {{.*}} @_ZN5empty2T76StructD2Ev(
 // CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 8)
-// CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 21)
+// CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 5)
 // CHECK-NEXT:    ret void
 
 namespace T8 {
@@ -160,7 +158,7 @@
 } // namespace T9
 // CHECK-LABEL: define {{.*}} @_ZN5empty2T96StructD2Ev(
 // CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 12)
-// CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 21)
+// CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 1)
 // CHECK-NEXT:    ret void
 
 namespace T10 {
@@ -190,7 +188,6 @@
 } // namespace T11
 // CHECK-LABEL: define {{.*}} @_ZN5empty3T116StructD2Ev(
 // CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 16)
-// CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 24)
 // CHECK-NEXT:    ret void
 
 namespace T12 {
@@ -317,6 +314,7 @@
 static_assert(sizeof(Struct) == 24);
 } // namespace T8
 // CHECK-LABEL: define {{.*}} @_ZN17empty_non_trivial2T86StructD2Ev(
+// CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 8)
 // CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 5)
 // CHECK-NEXT:    ret void
 
@@ -346,6 +344,7 @@
 static_assert(sizeof(Struct) == 24);
 } // namespace T10
 // CHECK-LABEL: define {{.*}} @_ZN17empty_non_trivial3T106StructD2Ev(
+// CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 12)
 // CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 1)
 // CHECK-NEXT:    ret void
 
@@ -375,4 +374,5 @@
 } // namespace T12
 } // namespace empty_non_trivial
 // CHECK-LABEL: define {{.*}} @_ZN17empty_non_trivial3T126StructD2Ev(
+// CHECK:         call void @__sanitizer_dtor_callback(i8* {{.*}}, i64 16)
 // CHECK:         ret void
Index: clang/lib/CodeGen/CGClass.cpp
===================================================================
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -1700,12 +1700,23 @@
         return FieldHasTrivialDestructorBody(Context, F);
       };
 
+      auto IsZeroSize = [&](const FieldDecl *F) {
+        return F->isZeroSize(Context);
+      };
+
+      // Poison blocks of fields with trivial destructors making sure that block
+      // begin and end do not point to zero-sized fields. They don't have
+      // correct offsets so can't be used to calculate poisoning range.
       for (auto It = Fields.begin(); It != Fields.end();) {
-        It = std::find_if(It, Fields.end(), IsTrivial);
+        It = std::find_if(It, Fields.end(), [&](const FieldDecl *F) {
+          return IsTrivial(F) && !IsZeroSize(F);
+        });
         if (It == Fields.end())
           break;
         auto Start = It++;
-        It = std::find_if_not(It, Fields.end(), IsTrivial);
+        It = std::find_if(It, Fields.end(), [&](const FieldDecl *F) {
+          return !IsTrivial(F) && !IsZeroSize(F);
+        });
 
         PoisonMembers(CGF, (*Start)->getFieldIndex(),
                       It == Fields.end() ? -1 : (*It)->getFieldIndex());


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D92727.309759.patch
Type: text/x-patch
Size: 4380 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201206/f4551ed6/attachment.bin>