[libcxx-commits] [PATCH] D132090: [1a/3][ASan][compiler-rt] API for double ended containers

[Vitaly Buka via Phabricator via libcxx-commits]

vitalybuka requested changes to this revision.
vitalybuka added a comment.
This revision now requires changes to proceed.

Please confirm that you have no committer access.
If so after the next your update, I will cleanup nits, if any, accept and land the patch.

Please make sure that check-sanitizer and check-asan pass.



================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:399
+// or end items of such a container
+void __sanitizer_annotate_double_ended_contiguous_container_front(
+    const void *storage_beg_p, const void *storage_end_p,
----------------
both non interface "static void"


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:401
+    const void *storage_beg_p, const void *storage_end_p,
+    const void *old_container_beg_p, const void *old_container_end_p,
+    const void *new_container_beg_p, const void *new_container_end_p) {
----------------
we don't use it old_container_end_p, new_container_end_p here, 
only for reporting

lets's make non-interface functions all uptr params, remove unneded ones and move the following block into interface function


```
  // Unchecked argument requirements:
  // During unpoisoning memory of empty container (before first element is
  // added):
  // - old_container_end_p == old_container_beg_p
  // During poisoning after last element was removed:
  // - new_container_beg_p == container_end_p
  if (!flags()->detect_container_overflow)
    return;
  VPrintf(2, "de_contiguous_container (front): %p %p %p %p %p\n", storage_beg_p,
          storage_end_p, new_container_beg_p, old_container_beg_p,
          old_container_end_p);
  uptr storage_beg = reinterpret_cast<uptr>(storage_beg_p);
  uptr storage_end = reinterpret_cast<uptr>(storage_end_p);
  uptr old_container_beg =
      reinterpret_cast<uptr>(old_container_beg_p);  // old container beginning
  uptr old_container_end =
      reinterpret_cast<uptr>(old_container_end_p);  // container ending
  uptr new_container_beg =
      reinterpret_cast<uptr>(new_container_beg_p);  // new container beginning
  uptr new_container_end =
      reinterpret_cast<uptr>(new_container_end_p);  // new container ending

  uptr granularity = ASAN_SHADOW_GRANULARITY;
  if (!((storage_beg <= new_container_beg &&
         new_container_beg <= storage_end) &&
        (storage_beg <= old_container_beg &&
         old_container_beg <= storage_end) &&
        (old_container_beg <= old_container_end &&
         old_container_end <= storage_end) &&
        IsAligned(storage_beg, granularity) &&
        old_container_end_p == new_container_end_p)) {
    GET_STACK_TRACE_FATAL_HERE;
    ReportBadParamsToAnnotateDoubleEndedContiguousContainer(
        storage_beg, storage_end, old_container_beg, old_container_end,
        new_container_beg, new_container_end, &stack);
  }
  CHECK_LE(storage_end - storage_beg,
           FIRST_32_SECOND_64(1UL << 30, 1ULL << 40));  // Sanity check.
```



================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:463
+                   granularity))  // is empty && ends in the middle of a block
+      *(u8 *)MemToShadow(c) = static_cast<u8>(old_container_end - c);
+
----------------
I assume after removing unneeded vars, we will have only old_container_end -> container_end


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:481
+    PoisonShadow(a, c - a, kAsanContiguousContainerOOBMagic);
+    if (new_container_beg == old_container_end &&
+        !IsAligned(new_container_beg,
----------------
if either body or condition are multi-line, please wrap the body with {}


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:552
+    PoisonShadow(a2, c2 - a2, kAsanContiguousContainerOOBMagic);
+    if (!IsAligned(new_container_end,
+                   granularity)) {  // Starts in the middle of the block
----------------
AddrIsAlignedByGranularity ,everywhere


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:562
+// Annotates a double ended contiguous memory area like std::deque's chunk.
+void __sanitizer_annotate_double_ended_contiguous_container(
+    const void *storage_beg_p, const void *storage_end_p,
----------------
new line after the function


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:566
+    const void *new_container_beg_p, const void *new_container_end_p) {
+  if (old_container_beg_p != new_container_beg_p)
+    __sanitizer_annotate_double_ended_contiguous_container_front(
----------------
we don't support !IsAligned(beg)
but please comment that we don't support !IsAligned(end) which shares the tail with something else

I assume we are not looking for intra object deque?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132090/new/

https://reviews.llvm.org/D132090