[asan] Clean up some confusing code in `test/asan/TestCases/Darwin/segv_read_write.c`

[delcypher]

Summary

[asan] Clean up some confusing code in
test/asan/TestCases/Darwin/segv_read_write.c

• The fd arg passed to mmap() should be -1. It is not definedwhat passing 0 does on Darwin.

• The comment about the shadow memory doesn't make any sense to me, so I'm removing it.

[delcypher]

@aizatsky Any comments on this? It appears you are the author of this test, so I'd like to make sure you're happy with the change.

[eugenis]

The comment makes sense to me. Writes are instrumented with reads from shadow in ASan. Therefore a write to addr in shadow will result in a read from shadow(addr), which is located in the mprotect-ed shadow gap, and will be reported as a read SEGV, not a write SEGV.

mmap() argument change looks fine.

[delcypher]

In D44579#1044875, @eugenis wrote:

The comment makes sense to me. Writes are instrumented with reads from shadow in ASan. Therefore a write to addr in shadow will result in a read from shadow(addr), which is located in the mprotect-ed shadow gap, and will be reported as a read SEGV, not a write SEGV.

mmap() argument change looks fine.

Thanks for that explanation. That makes more sense now. I'm going to integrate your explanation into the comment and merge.

[delcypher]

In D44579#1160019, @delcypher wrote:

In D44579#1044875, @eugenis wrote:

The comment makes sense to me. Writes are instrumented with reads from shadow in ASan. Therefore a write to addr in shadow will result in a read from shadow(addr), which is located in the mprotect-ed shadow gap, and will be reported as a read SEGV, not a write SEGV.

mmap() argument change looks fine.

Thanks for that explanation. That makes more sense now. I'm going to integrate your explanation into the comment and merge.

Actually now I'm completely confused. Running the test on macOS I observe that

• The Read() function triggers: The signal is caused by a READ memory access. The access is on the memory location p which indicates the access check instrumentation that reads the shadow passed.
• The Write() function triggers: The signal is caused by a WRITE memory access. The access is on the memory location p which indicates the access check instrumentation that reads the shadow passed.
• The Read() function has instrumentation to read from the shadow to check the access is okay. There are no writes to the shadow in this function.
• The Write() function has instrumentation to read from the shadow to check the access is okay. There are no writes to the shadow in this function.
• We do intercept mmap() but it doesn't look like we update the shadow memory in the interceptor.
• We don't intercept munmap() so we aren't updating the shadow here either.

In light of this the comments do not make sense.

• There are no writes to the shadow (okay there is some instrumentation in main() that looks like its setting up the shadow stack but that seems unrelated). Where are the writes to the shadow mentioned in the comments coming from?
• The comments imply doing a write will result in reporting a read in the SEGV handler. That's not what the test is checking for and is not what I observe when running it.

[kubamracek]

In D44579#1160019, @delcypher wrote:

In D44579#1044875, @eugenis wrote:

The comment makes sense to me. Writes are instrumented with reads from shadow in ASan. Therefore a write to addr in shadow will result in a read from shadow(addr), which is located in the mprotect-ed shadow gap, and will be reported as a read SEGV, not a write SEGV.

What I'm reading from the test is that we're accessing a normal address in normal app part of memory. This is backed by an ordinary byte in the shadow memory. The gap is not involved here at all. We know whether the faulty access is a read or write based because the instrumentation passes this information to __asan_report_error. I don't see how the comment is helpful.

[kubamracek]

Actually, it's even simpler that that. The access is always valid (from the shadow's perspective). We're just trapping on a regular SEGV and the signal handler gets the information whether this is a read or a write.

[delcypher]

In D44579#1160273, @kubamracek wrote:

Actually, it's even simpler that that. The access is always valid (from the shadow's perspective). We're just trapping on a regular SEGV and the signal handler gets the information whether this is a read or a write.

Yes that's what I observed. Presumably we don't update the shadow for mmap() and munmap() calls because the OS will handle bad accesses for us by trapping?

test/asan/TestCases/Darwin/segv_read_write.c

 // RUN: %clangxx_asan -std=c++11 -O0 %s -o %t
 // RUN: not %run %t       2>&1 | FileCheck %s --check-prefix=READ
 // RUN: not %run %t write 2>&1 | FileCheck %s --check-prefix=WRITE
 // REQUIRES: x86-target-arch
 
 #include <sys/mman.h>
 
 static volatile int sink;
 __attribute__((noinline)) void Read(int *ptr) { sink = *ptr; }
 __attribute__((noinline)) void Write(int *ptr) { *ptr = 0; }
 int main(int argc, char **argv) {
-  // Writes to shadow are detected as reads from shadow gap (because of how the
-  // shadow mapping works). This is kinda hard to fix. Test a random address in
-  // the application part of the address space.
   void *volatile p =
-      mmap(nullptr, 4096, PROT_READ, MAP_PRIVATE | MAP_ANON, 0, 0);
+      mmap(nullptr, 4096, PROT_READ, MAP_PRIVATE | MAP_ANON, -1, 0);
   munmap(p, 4096);
   if (argc == 1)
     Read((int *)p);
   else
     Write((int *)p);
 }
 // READ: AddressSanitizer: SEGV on unknown address
 // READ: The signal is caused by a READ memory access.
 // WRITE: AddressSanitizer: SEGV on unknown address
 // WRITE: The signal is caused by a WRITE memory access.