[ASan] Fixed a reporting bug in (load|store)N functions which would print unknown-crash instead of the proper error message when a the data access is unaligned.

[kda]

A couple of nits.

compiler-rt/test/asan/TestCases/load_and_store_n.cpp

34–35

nit: maybe move below include

59

prepend 'res = ' ?

[kstoimenov]

Moved vars below include.

[kstoimenov]

compiler-rt/test/asan/TestCases/load_and_store_n.cpp

59

Store is a void function.

[eugenis]

compiler-rt/test/asan/TestCases/load_and_store_n.cpp

30

Could you tighten this check a little? Ex. you could print the expected "bad access" address in the test code, and then verify that it matches the address printed by asan.

Also an interesting edge case to test would be when the access is entirely outside of the global (ex. (char*)&G + 9).

[kstoimenov]

Addressed comments.

[eugenis]

LGTM w/ nits

compiler-rt/test/asan/TestCases/load_and_store_n.cpp

37

Could you also extend this check? I imagine this would say "0 bytes to the right" vs "1 bytes to the right" depending on the case.

[kstoimenov]

Extended checks to 0 and 1 cases.

[kstoimenov]

Tests are against the IR instead of assembly.

[kstoimenov]

Tests are against the IR instead of assembly - for real.

[kstoimenov]

Fixed tests.

[eugenis]

android_compile script is confused by -emit-llvm it looks like. I'd just remove the IR checks from the test.

+ /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/sanitizer_common/android_commands/android_compile.py /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm_build64/bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only --target=aarch64-linux-android24 --sysroot=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -B/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta -stdlib=libc++ -fuse-ld=lld -shared-libasan -O2 -fsanitize-address-outline-instrumentation /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/load_and_store_n.cpp -o - -S -emit-llvm
+ FileCheck /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/load_and_store_n.cpp --check-prefix=CHECK_REGULAR_LOAD_STORE
clang-15: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
Traceback (most recent call last):
  File "/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/sanitizer_common/android_commands/android_compile.py", line 35, in <module>
    os.rename(output, output + '.real')
OSError: [Errno 2] No such file or directory

[kstoimenov]

Sent D124030 to remove the checks.

[vitalybuka]

compiler-rt/test/asan/TestCases/load_and_store_n.cpp

30

Should we have IR test on the clang side?
GCC uses compiler-rt as well

[Enna1]

Can we do this on the ASan error report side (i.e. compiler-rt/lib/asan/asan_errors.cpp) .

diff --git a/compiler-rt/lib/asan/asan_errors.cpp b/compiler-rt/lib/asan/asan_errors.cpp
index a22bf130d823..2cdd13df6c2a 100644
--- a/compiler-rt/lib/asan/asan_errors.cpp
+++ b/compiler-rt/lib/asan/asan_errors.cpp
@@ -409,8 +409,10 @@ ErrorGeneric::ErrorGeneric(u32 tid, uptr pc_, uptr bp_, uptr sp_, uptr addr,
     bug_descr = "unknown-crash";
     if (AddrIsInMem(addr)) {
       u8 *shadow_addr = (u8 *)MemToShadow(addr);
-      // If we are accessing 16 bytes, look at the second shadow byte.
-      if (*shadow_addr == 0 && access_size > ASAN_SHADOW_GRANULARITY)
+      // If we are accessing 16 bytes or in unaligned accesses, look at the
+      // second shadow byte.
+      if (*shadow_addr == 0 && (access_size > ASAN_SHADOW_GRANULARITY ||
+                                (addr & (ASAN_SHADOW_GRANULARITY - 1)) != 0))
         shadow_addr++;
       // If we are in the partial right redzone, look at the next shadow byte.
       if (*shadow_addr > 0 && *shadow_addr < 128) shadow_addr++;

In this way, the error report of the test load_and_store_n.cpp (e.g. %run %t A) will like:

==2884775==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000000545bc7 at pc 0x00000050bf95 bp 0x7ffcdcbcb350 sp 0x7ffcdcbcb338
READ of size 2 at 0x000000545bc7 thread T0
...
0x000000545bc8 is located 0 bytes to the right of global variable 'mem' defined in 'load_and_store_n.cpp:46:16' (0x545bc0) of size 8
SUMMARY: AddressSanitizer: global-buffer-overflow ./load_and_store_n.cpp:51:3 in UNALIGNED_LOAD(void const*)
Shadow bytes around the buggy address:
  0x0000800a0b20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0000800a0b30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0000800a0b40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0000800a0b50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0000800a0b60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0000800a0b70: 00 00 00 00 00 f9 f9 f9[00]f9 f9 f9 f9 f9 f9 f9
  0x0000800a0b80: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
  0x0000800a0b90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0000800a0ba0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0000800a0bb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0000800a0bc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

This way the access start address (0x000000545bc7) in this report is more readable.

compiler-rt/lib/asan/asan_rtl.cpp

 ASAN_MEMORY_ACCESS_CALLBACK(store, true, 2)
 ASAN_MEMORY_ACCESS_CALLBACK(store, true, 4)
 ASAN_MEMORY_ACCESS_CALLBACK(store, true, 8)
 ASAN_MEMORY_ACCESS_CALLBACK(store, true, 16)
 
 extern "C"
 NOINLINE INTERFACE_ATTRIBUTE
 void __asan_loadN(uptr addr, uptr size) {
-  if (__asan_region_is_poisoned(addr, size)) {
+  if ((addr = __asan_region_is_poisoned(addr, size))) {
     GET_CALLER_PC_BP_SP;
     ReportGenericError(pc, bp, sp, addr, false, size, 0, true);
   }
 }
 
 extern "C"
 NOINLINE INTERFACE_ATTRIBUTE
 void __asan_exp_loadN(uptr addr, uptr size, u32 exp) {
-  if (__asan_region_is_poisoned(addr, size)) {
+  if ((addr = __asan_region_is_poisoned(addr, size))) {
     GET_CALLER_PC_BP_SP;
     ReportGenericError(pc, bp, sp, addr, false, size, exp, true);
   }
 }
 
 extern "C"
 NOINLINE INTERFACE_ATTRIBUTE
 void __asan_loadN_noabort(uptr addr, uptr size) {
-  if (__asan_region_is_poisoned(addr, size)) {
+  if ((addr = __asan_region_is_poisoned(addr, size))) {
     GET_CALLER_PC_BP_SP;
     ReportGenericError(pc, bp, sp, addr, false, size, 0, false);
   }
 }
 
 extern "C"
 NOINLINE INTERFACE_ATTRIBUTE
 void __asan_storeN(uptr addr, uptr size) {
-  if (__asan_region_is_poisoned(addr, size)) {
+  if ((addr = __asan_region_is_poisoned(addr, size))) {
     GET_CALLER_PC_BP_SP;
     ReportGenericError(pc, bp, sp, addr, true, size, 0, true);
   }
 }
 
 extern "C"
 NOINLINE INTERFACE_ATTRIBUTE
 void __asan_exp_storeN(uptr addr, uptr size, u32 exp) {
-  if (__asan_region_is_poisoned(addr, size)) {
+  if ((addr = __asan_region_is_poisoned(addr, size))) {
     GET_CALLER_PC_BP_SP;
     ReportGenericError(pc, bp, sp, addr, true, size, exp, true);
   }
 }
 
 extern "C"
 NOINLINE INTERFACE_ATTRIBUTE
 void __asan_storeN_noabort(uptr addr, uptr size) {
-  if (__asan_region_is_poisoned(addr, size)) {
+  if ((addr = __asan_region_is_poisoned(addr, size))) {
     GET_CALLER_PC_BP_SP;
     ReportGenericError(pc, bp, sp, addr, true, size, 0, false);
   }
 }
 
 // Force the linker to keep the symbols for various ASan interface functions.
 // We want to keep those in the executable in order to let the instrumented
 // dynamic libraries access the symbol even if it is not used by the executable

compiler-rt/test/asan/TestCases/load_and_store_n.cpp

+// CHECK_REGULAR_LOAD_STORE: call void @__asan_loadN
+// CHECK_REGULAR_LOAD_STORE: call void @__asan_storeN
+// RUN: %clangxx_asan -O2 -fsanitize-address-outline-instrumentation %s -o %t
+// RUN: %clangxx_asan -O2 -fsanitize-address-outline-instrumentation %s -o - -S \
+// RUN:   -emit-llvm | FileCheck %s --check-prefix=CHECK_REGULAR_LOAD_STORE
+// RUN: not %run %t A 2>&1 | FileCheck %s --check-prefix=CHECK_0_BYTES
+// RUN: not %run %t B 2>&1 | FileCheck %s --check-prefix=CHECK_0_BYTES
+// RUN: not %run %t C 2>&1 | FileCheck %s --check-prefix=CHECK_1_BYTES
+// RUN: not %run %t D 2>&1 | FileCheck %s --check-prefix=CHECK_1_BYTES
+
+// CHECK_NOABORT_LOAD_STORE: call void @__asan_loadN_noabort
+// CHECK_NOABORT_LOAD_STORE: call void @__asan_storeN_noabort
+// RUN: %clangxx_asan -O2 -fsanitize-address-outline-instrumentation %s -o %t \
+// RUN:   -mllvm -asan-recover=1
+// RUN: %clangxx_asan -O2 -fsanitize-address-outline-instrumentation %s -o - -S \
+// RUN:   -mllvm -asan-recover=1 -emit-llvm \
+// RUN:   | FileCheck %s --check-prefix=CHECK_NOABORT_LOAD_STORE
+// RUN: not %run %t A 2>&1 | FileCheck %s --check-prefix=CHECK_0_BYTES
+// RUN: not %run %t B 2>&1 | FileCheck %s --check-prefix=CHECK_0_BYTES
+// RUN: not %run %t C 2>&1 | FileCheck %s --check-prefix=CHECK_1_BYTES
+// RUN: not %run %t D 2>&1 | FileCheck %s --check-prefix=CHECK_1_BYTES
+
+// CHECK_EXP_LOAD_STORE: call void @__asan_exp_loadN
+// CHECK_EXP_LOAD_STORE: call void @__asan_exp_storeN
+// RUN: %clangxx_asan -O2 -fsanitize-address-outline-instrumentation %s -o %t \
+// RUN:   -mllvm -asan-force-experiment=42
+// RUN: %clangxx_asan -O2 -fsanitize-address-outline-instrumentation %s -o - -S \
+// RUN:   -mllvm -asan-force-experiment=42 -emit-llvm \
+// RUN:   | FileCheck %s --check-prefix=CHECK_EXP_LOAD_STORE
+// RUN: not %run %t A 2>&1 | FileCheck %s --check-prefix=CHECK_0_BYTES
+// RUN: not %run %t B 2>&1 | FileCheck %s --check-prefix=CHECK_0_BYTES
+// RUN: not %run %t C 2>&1 | FileCheck %s --check-prefix=CHECK_1_BYTES
+// RUN: not %run %t D 2>&1 | FileCheck %s --check-prefix=CHECK_1_BYTES
+
+// CHECK_0_BYTES: ERROR: AddressSanitizer: global-buffer-overflow on address [[ADDR:.*]] at
+// CHECK_0_BYTES: [[ADDR]] is located 0 bytes to the right
+
+// CHECK_1_BYTES: ERROR: AddressSanitizer: global-buffer-overflow on address [[ADDR:.*]] at
+// CHECK_1_BYTES: [[ADDR]] is located 1 bytes to the right
+
+#include <sanitizer/asan_interface.h>
+
+#include <stdlib.h>
+#include <string.h>
+
+static int64_t mem = -1;
+static int64_t *volatile G = &mem;
+
+inline uint16_t UNALIGNED_LOAD(const void *p) {
+  uint16_t data;
+  memcpy(&data, p, sizeof data);
+  return data;
+}
+
+inline void UNALIGNED_STORE(uint16_t data, void *p) {
+  memcpy(p, &data, sizeof data);
+}
+
+int main(int argc, char **argv) {
+  if (argc != 2)
+    return 1;
+  int res = 1;
+  switch (argv[1][0]) {
+  case 'A':
+    res = UNALIGNED_LOAD(reinterpret_cast<char *>(G) + 7);
+    break;
+  case 'B':
+    UNALIGNED_STORE(0, reinterpret_cast<char *>(G) + 7);
+    break;
+  case 'C':
+    res = UNALIGNED_LOAD(reinterpret_cast<char *>(G) + 9);
+    break;
+  case 'D':
+    UNALIGNED_STORE(0, reinterpret_cast<char *>(G) + 9);
+    break;
+  }
+  return res;
+}