Skip to content

Conversation

@vitalybuka
Copy link
Collaborator

The comment stated that it's slow, but likely it's a deadlock,
as write can be blocked.

Also we can't be sure that page_size * 10 is appropriate size.

Still most likely this is NFC, as the max size we use is 32,
and should fit in any buffer.

Created using spr 1.3.4
@llvmbot
Copy link
Member

llvmbot commented Oct 17, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Vitaly Buka (vitalybuka)

Changes

The comment stated that it's slow, but likely it's a deadlock,
as write can be blocked.

Also we can't be sure that page_size * 10 is appropriate size.

Still most likely this is NFC, as the max size we use is 32,
and should fit in any buffer.


Full diff: https://github.com/llvm/llvm-project/pull/112665.diff

2 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp (+34-18)
  • (modified) compiler-rt/lib/sanitizer_common/tests/sanitizer_posix_test.cpp (+11)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
index 9ffb36f812c45d..0f8fdd4487efb6 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
@@ -288,26 +288,42 @@ bool SignalContext::IsStackOverflow() const {
 
 #endif  // SANITIZER_GO
 
+static void SetNonBlock(int fd) {
+  int res = fcntl(fd, F_GETFL, 0);
+  CHECK(!internal_iserror(res, nullptr));
+
+  res |= O_NONBLOCK;
+  res = fcntl(fd, F_SETFL, res);
+  CHECK(!internal_iserror(res, nullptr));
+}
+
 bool IsAccessibleMemoryRange(uptr beg, uptr size) {
-  uptr page_size = GetPageSizeCached();
-  // Checking too large memory ranges is slow.
-  CHECK_LT(size, page_size * 10);
-  int sock_pair[2];
-  if (pipe(sock_pair))
-    return false;
-  uptr bytes_written =
-      internal_write(sock_pair[1], reinterpret_cast<void *>(beg), size);
-  int write_errno;
-  bool result;
-  if (internal_iserror(bytes_written, &write_errno)) {
-    CHECK_EQ(EFAULT, write_errno);
-    result = false;
-  } else {
-    result = (bytes_written == size);
+  for (uptr to_write = size; to_write;) {
+    // `read` from `sock_pair[0]` into a dummy buffer to free up the pipe buffer
+    // for more `write` is slower than just recreating a pipe.
+    int sock_pair[2];
+    if (pipe(sock_pair))
+      return false;
+
+    auto cleanup = at_scope_exit([&]() {
+      internal_close(sock_pair[0]);
+      internal_close(sock_pair[1]);
+    });
+
+    SetNonBlock(sock_pair[1]);
+
+    int write_errno;
+    uptr bytes_written =
+        internal_write(sock_pair[1], reinterpret_cast<char *>(beg), to_write);
+    if (internal_iserror(bytes_written, &write_errno)) {
+      CHECK_EQ(EFAULT, write_errno);
+      return false;
+    }
+    beg += bytes_written;
+    to_write -= bytes_written;
   }
-  internal_close(sock_pair[0]);
-  internal_close(sock_pair[1]);
-  return result;
+
+  return true;
 }
 
 void PlatformPrepareForSandboxing(void *args) {
diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_posix_test.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_posix_test.cpp
index bed19d15a8ec77..9feb22221f005e 100644
--- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_posix_test.cpp
+++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_posix_test.cpp
@@ -82,6 +82,17 @@ TEST(SanitizerCommon, IsAccessibleMemoryRange) {
   munmap((void *)mem, 3 * page_size);
 }
 
+TEST(SanitizerCommon, IsAccessibleMemoryRangeLarge) {
+  const int size = GetPageSize() * 10000;
+
+  uptr mem = (uptr)mmap(0, size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON,
+                        -1, 0);
+
+  EXPECT_TRUE(IsAccessibleMemoryRange(mem, size));
+
+  munmap((void *)mem, size);
+}
+
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_POSIX

Created using spr 1.3.4
@vitalybuka vitalybuka requested a review from fmayer October 17, 2024 07:22
Created using spr 1.3.4
@fmayer
Copy link
Contributor

fmayer commented Oct 17, 2024

FYI, If you want to optimize this on Linux you could probably also use sendfile or splice there. Wrong CL :)

Created using spr 1.3.4
Created using spr 1.3.4
@vitalybuka vitalybuka requested a review from fmayer October 17, 2024 17:45
Created using spr 1.3.4
@vitalybuka vitalybuka requested a review from fmayer October 17, 2024 17:52
Created using spr 1.3.4
@vitalybuka vitalybuka requested a review from fmayer October 17, 2024 18:17
Copy link
Contributor

@fmayer fmayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@vitalybuka vitalybuka merged commit 7086584 into main Oct 17, 2024
7 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/sanitizer-large-range-support-in-isaccessiblememoryrange branch October 17, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants