From c9e82887bd8ec0f561a1e9a34d9fce209e12376d Mon Sep 17 00:00:00 2001
From: Michael Lippautz <mlippautz@chromium.org>
Date: Fri, 7 May 2021 00:05:55 +0200
Subject: [PATCH] cppgc: Allow ASAN-safe memset in SetMemoryInaccessible()

The application may itself change ASAN poisoning which conflicts with
the memset() right before poisoning memory.

This is relevant for destructors but also when invoking Resize() on an
object that uses ASAN container annotations. Annotations are hard to
adjust for the embedder as it is not clear upfront whether the call will
succeed.

Bug: chromium:1056170
Change-Id: I7f719e4130ba6149494a45f220a341658970bc6f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2878733
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74431}
---
 BUILD.gn                                      |  1 +
 src/heap/cppgc/memory.cc                      | 22 ++++++++++++++++++
 src/heap/cppgc/memory.h                       |  5 +++-
 src/heap/cppgc/sweeper.cc                     |  3 ---
 .../heap/cppgc/sanitizer-unittest.cc          | 23 +++++++++++++++++++
 5 files changed, 50 insertions(+), 4 deletions(-)
 create mode 100644 src/heap/cppgc/memory.cc

diff --git a/BUILD.gn b/BUILD.gn
index 78d6ef8b21..2b6af4e06d 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -5009,6 +5009,7 @@ v8_source_set("cppgc_base") {
     "src/heap/cppgc/marking-visitor.h",
     "src/heap/cppgc/marking-worklists.cc",
     "src/heap/cppgc/marking-worklists.h",
+    "src/heap/cppgc/memory.cc",
     "src/heap/cppgc/memory.h",
     "src/heap/cppgc/metric-recorder.h",
     "src/heap/cppgc/name-trait.cc",
diff --git a/src/heap/cppgc/memory.cc b/src/heap/cppgc/memory.cc
new file mode 100644
index 0000000000..aa3baeaa8a
--- /dev/null
+++ b/src/heap/cppgc/memory.cc
@@ -0,0 +1,22 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/heap/cppgc/memory.h"
+
+#include <cstddef>
+
+#include "src/heap/cppgc/globals.h"
+
+namespace cppgc {
+namespace internal {
+
+void NoSanitizeMemset(void* address, char c, size_t bytes) {
+  volatile Address base = reinterpret_cast<Address>(address);
+  for (size_t i = 0; i < bytes; ++i) {
+    base[i] = c;
+  }
+}
+
+}  // namespace internal
+}  // namespace cppgc
diff --git a/src/heap/cppgc/memory.h b/src/heap/cppgc/memory.h
index e03ca7b949..d31af33ee3 100644
--- a/src/heap/cppgc/memory.h
+++ b/src/heap/cppgc/memory.h
@@ -16,6 +16,9 @@
 namespace cppgc {
 namespace internal {
 
+V8_NOINLINE DISABLE_ASAN void NoSanitizeMemset(void* address, char c,
+                                               size_t bytes);
+
 inline void ZapMemory(void* address, size_t size) {
   // The lowest bit of the zapped value should be 0 so that zapped object are
   // never viewed as fully constructed objects.
@@ -53,7 +56,7 @@ V8_INLINE void SetMemoryInaccessible(void* address, size_t size) {
 
 #elif defined(V8_USE_ADDRESS_SANITIZER)
 
-  memset(address, 0, size);
+  NoSanitizeMemset(address, 0, size);
   ASAN_POISON_MEMORY_REGION(address, size);
 
 #elif DEBUG
diff --git a/src/heap/cppgc/sweeper.cc b/src/heap/cppgc/sweeper.cc
index 456538744b..a036551104 100644
--- a/src/heap/cppgc/sweeper.cc
+++ b/src/heap/cppgc/sweeper.cc
@@ -161,9 +161,6 @@ class DeferredFinalizationBuilder final {
       result_.unfinalized_objects.push_back({header});
       found_finalizer_ = true;
     } else {
-      // Unmarked memory may have been poisoned. In the non-concurrent case this
-      // is taken care of by finalizing a header.
-      ASAN_UNPOISON_MEMORY_REGION(header, size);
       SetMemoryInaccessible(header, size);
     }
   }
diff --git a/test/unittests/heap/cppgc/sanitizer-unittest.cc b/test/unittests/heap/cppgc/sanitizer-unittest.cc
index 4f6f853800..687904b825 100644
--- a/test/unittests/heap/cppgc/sanitizer-unittest.cc
+++ b/test/unittests/heap/cppgc/sanitizer-unittest.cc
@@ -4,6 +4,7 @@
 
 #include "include/cppgc/allocation.h"
 #include "src/base/macros.h"
+#include "src/base/sanitizer/asan.h"
 #include "test/unittests/heap/cppgc/tests.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -32,5 +33,27 @@ TEST_F(LsanTest, LeakDetectionDoesNotFindMemoryRetainedFromManaged) {
 
 #endif  // LEAK_SANITIZER
 
+#ifdef V8_USE_ADDRESS_SANITIZER
+
+using AsanTest = testing::TestWithHeap;
+
+class ObjectPoisoningInDestructor final
+    : public GarbageCollected<ObjectPoisoningInDestructor> {
+ public:
+  ~ObjectPoisoningInDestructor() {
+    ASAN_POISON_MEMORY_REGION(this, sizeof(ObjectPoisoningInDestructor));
+  }
+  void Trace(cppgc::Visitor*) const {}
+
+  void* dummy{0};
+};
+
+TEST_F(AsanTest, ObjectPoisoningInDestructor) {
+  MakeGarbageCollected<ObjectPoisoningInDestructor>(GetAllocationHandle());
+  PreciseGC();
+}
+
+#endif  // V8_USE_ADDRESS_SANITIZER
+
 }  // namespace internal
 }  // namespace cppgc