[ASAN] Create a new UseAfterScopeError

[marxin]

Summary

I've been currently working use-after-scope sanitization in GCC. We see optimization opportunity to rewrite local variables (when poisoned) to SSA_NAMEs
where a usage of such SSA_NAME would lead to use-after-scope error. Compared to the current implementation, there SSA_NAMEs (corresponding to a local variable)
does not have a stack store and thus there's no reserved slot in shadow memory. For these I would like to add a new API function builtin_asan_report_use_after_scope.

Let's consider following sample:
int
main (void)
{

char *ptr;
{
  char my_char;
  ptr = &my_char;
}

return *ptr;

}

which can be transformed to:

main ()
{

char my_char;
int _4;

<bb 2>:
__builtin___asan_report_use_after_scope ("my_char", 1);
_4 = (int) my_char_5(D);
return _4;

}

Corresponding run-time error looks as follows:

16049==ERROR: AddressSanitizer: stack-use-after-scope at pc 0x000000400794 bp 0x000000000001 sp 0x0000004005f3

ACCESS of size 1 for variable 'my_char' thread T0

#0 0x4005f2 in main (/tmp/a.out+0x4005f2)
#1 0x7f883337e290 in __libc_start_main (/lib64/libc.so.6+0x20290)
#2 0x400649 in _start (/tmp/a.out+0x400649)

SUMMARY: AddressSanitizer: stack-use-after-scope (/tmp/a.out+0x4005f2) in main

16049==ABORTING

[filcab]

I'll let @kcc (or @vitalybuka ?) chime in on the API before an LGTM. I don't see a big problem on my part, if you don't have memory to back that variable. But I don't know much about gcc internals.

compiler-rt/lib/asan/asan_errors.cc

289

clang-format

295

Can the compiler know if it's a read or write in many cases? Or does it now have that information?

297

Does clang-format agree with this?

compiler-rt/lib/asan/asan_errors.h

313

Please run clang-format on this.

348

Please don't change enum values unless there's a huge reason for it. Place this one after Generic.

compiler-rt/lib/asan/asan_report.cc

358

No sense having this comment.

compiler-rt/lib/asan/asan_rtl.cc

258

Why?

[vitalybuka]

compiler-rt/lib/asan/asan_errors.cc

295

Would be nice to have report as close to regular one as possible.

[kcc]

Hm... I don't understand this change.
If you are adding a call to __asan_report_use_after_scope it means that you can statically (at compile-time) see the problem,
which in turn means that you can issue a warning/error at compile time and not bother with asan.

[marxin]

In D26965#608664, @kcc wrote:

Hm... I don't understand this change.
If you are adding a call to __asan_report_use_after_scope it means that you can statically (at compile-time) see the problem,
which in turn means that you can issue a warning/error at compile time and not bother with asan.

Yep, I know that at compile time that I emit a call to the new builtin. But similar to all other sanitizers, the code must be reachable at run-time.

[marxin]

I'm attaching a new version, where I fixed all clang-format issue, trailing include and the error is used just for READ operation.
The error message is very similar to the normal (no stack variables are displayed and address is not printed).

After the patch:

gcc -fsanitize=address -fsanitize-address-use-after-scope /tmp/use-after-scope-20.c  -O2 && ./a.out
=================================================================
==31148==ERROR: AddressSanitizer: stack-use-after-scope at pc 0x000000400764 bp 0x000000000004 sp 0x0000004005d3
READ of size 4 for variable 'my_char' thread T0
    #0 0x4005d2 in main (/tmp/a.out+0x4005d2)
    #1 0x7f7a1dafd290 in __libc_start_main (/lib64/libc.so.6+0x20290)
    #2 0x400629 in _start (/tmp/a.out+0x400629)

SUMMARY: AddressSanitizer: stack-use-after-scope (/tmp/a.out+0x4005d2) in main
==31148==ABORTING

Before:

gcc -fsanitize=address -fsanitize-address-use-after-scope /tmp/use-after-scope-20.c  -O0 && ./a.out > /tmp/0
=================================================================
==19162==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7ffc7b54ec10 at pc 0x000000400828 bp 0x7ffc7b54ebc0 sp 0x7ffc7b54ebb8
READ of size 4 at 0x7ffc7b54ec10 thread T0
    #0 0x400827 in main (/tmp/a.out+0x400827)
    #1 0x7fe10ceb5290 in __libc_start_main (/lib64/libc.so.6+0x20290)
    #2 0x400699 in _start (/tmp/a.out+0x400699)

Address 0x7ffc7b54ec10 is located in stack of thread T0 at offset 32 in frame
    #0 0x400756 in main (/tmp/a.out+0x400756)

  This frame has 1 object(s):
    [32, 36) 'my_char' <== Memory access at offset 32 is inside this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-scope (/tmp/a.out+0x400827) in main
Shadow bytes around the buggy address:
  0x10000f6a1d30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10000f6a1d40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10000f6a1d50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10000f6a1d60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10000f6a1d70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1
=>0x10000f6a1d80: f1 f1[f8]f2 f2 f2 f3 f3 f3 f3 00 00 00 00 00 00
  0x10000f6a1d90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10000f6a1da0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10000f6a1db0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10000f6a1dc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10000f6a1dd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==19162==ABORTING

[filcab]

Minor nits. Otherwise I see no problem with this, if @kcc is ok with adding the error.

libsanitizer/asan/asan_errors.h

313 ↗

(On Diff #80745)

Also define this after ErrorGeneric, making it the same order as the enum.
Same thing for the cc file.

[vitalybuka]

Yep, I know that at compile time that I emit a call to the new builtin. But similar to all other sanitizers, the code must be reachable at run-time.

My understanding is that it's always better to detect stuff in compile time than in runtime.
The goal of sanitizes is to detect bug which are impossible or very hard to detect at compile time.
Seems consistency is not good enough justification for hiding this bugs during compilation.

[kcc]

In D26965#618270, @filcab wrote:

Minor nits. Otherwise I see no problem with this, if @kcc is ok with adding the error.

kcc is not ok with this change, as he indicated previously. :(

[marxin]

In D26965#618369, @kcc wrote:

In D26965#618270, @filcab wrote:

Minor nits. Otherwise I see no problem with this, if @kcc is ok with adding the error.

kcc is not ok with this change, as he indicated previously. :(

I added comment which explains that even though we call __asan_report_use_after_scope function, the call still may be conditional. And possibly can't be never reached. I guess that's nature of sanitizers, where an undefined behavior must be triggered in run-time.

[kcc]

I am still not convinced this is useful, but if you want to convince me

• please add a test to this CL (in projects/compiler-rt/test/asan)

and we will discuss the test.

[marxin]

Updated:

• UseAfterScope::Print is put after ErrorGeneric::Print
• struct ErrorUseAfterScope : ErrorBase is after ErrorGeneric
• New test added: compiler-rt/test/asan/TestCases/use-after-scope-conditional.cc

[kcc]

The code in the example sucks so utterly that it should be fine to issue a compile-time warning on it.
And if you can't issue a warning, you also can't instrument the program in the suggested way.

compiler-rt/lib/asan/asan_errors.h

   void Print();
 };
 
+struct ErrorUseAfterScope : ErrorBase {
+  uptr pc, bp, sp;
+  const char *variable_name;
+  uptr variable_size;
+  // VS2013 doesn't implement unrestricted unions, so we need a trivial default
+  // constructor
+  ErrorUseAfterScope() = default;
+  ErrorUseAfterScope(u32 tid, uptr pc_, uptr bp_, uptr sp_,
+                     const char *variable_name_, uptr variable_size_)
+      : ErrorBase(tid),
+        pc(pc_),
+        bp(bp_),
+        sp(sp_),
+        variable_name(variable_name_),
+        variable_size(variable_size_) {}
+  void Print();
+};
+
 // clang-format off
 #define ASAN_FOR_EACH_ERROR_KIND(macro)         \
   macro(StackOverflow)                          \

   macro(BadParamsToAnnotateContiguousContainer) \
   macro(ODRViolation)                           \
   macro(InvalidPointerPair)                     \
-  macro(Generic)
+  macro(Generic)        \
+  macro(UseAfterScope)
 // clang-format on
 
 #define ASAN_DEFINE_ERROR_KIND(name) kErrorKind##name,

compiler-rt/lib/asan/asan_errors.cc

   PrintShadowMemoryForAddress(addr);
 }
 
+void ErrorUseAfterScope::Print() {
+  const char *bug_type = "stack-use-after-scope";
+  Decorator d;
+  Printf("%s", d.Warning());
+
+  Report(
+      "ERROR: AddressSanitizer: stack-use-after-scope at pc %p bp %p sp %p\n",
+      variable_name, variable_size, pc, bp, sp);
+  Printf("%s", d.EndWarning());
+  scariness.Print();
+
+  char tname[128];
+  Printf("READ of size %zu for variable '%s' thread T%d%s%s\n", variable_size,
+         variable_name, tid,
+         ThreadNameWithParenthesis(tid, tname, sizeof(tname)), d.EndAccess());
+
+  GET_STACK_TRACE_FATAL(pc, bp);
+  stack.Print();
+  ReportErrorSummary(bug_type, &stack);
+}
+
 }  // namespace __asan

compiler-rt/lib/asan/asan_report.h

 void ReportODRViolation(const __asan_global *g1, u32 stack_id1,
                         const __asan_global *g2, u32 stack_id2);
 
+void ReportUseAfterScope(const char *variable_name, uptr variable_size,
+                         bool fatal);
+
 // Mac-specific errors and warnings.
 void ReportMacMzReallocUnknown(uptr addr, uptr zone_ptr,
                                const char *zone_name,

compiler-rt/lib/asan/asan_report.cc

     return ReportInvalidPointerPair(pc, bp, sp, a1, a2);
   }
 }
+
+void ReportUseAfterScope(const char *variable_name, uptr variable_size,
+                         bool fatal) {
+  ScopedInErrorReport in_report (fatal);
+  GET_CALLER_PC_BP_SP;
+  ErrorUseAfterScope error(GetCurrentTidOrInvalid(), pc, bp, sp, variable_name,
+                           variable_size);
+  in_report.ReportError(error);
+}
+
 // ----------------------- Mac-specific reports ----------------- {{{1
 
 void ReportMacMzReallocUnknown(uptr addr, uptr zone_ptr, const char *zone_name,

compiler-rt/lib/asan/asan_rtl.cc

   }
 }
 
+extern "C"
+NOINLINE INTERFACE_ATTRIBUTE
+void __asan_report_use_after_scope(const char *variable_name,
+                                   uptr variable_size) {
+  ReportUseAfterScope(variable_name, variable_size, true);
+}
+
+extern "C"
+NOINLINE INTERFACE_ATTRIBUTE
+void __asan_report_use_after_scope_noabort(const char *variable_name,
+                                           uptr variable_size) {
+  ReportUseAfterScope(variable_name, variable_size, 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/use-after-scope-conditional.cc

+// RUN: %clangxx_asan -O2 -fsanitize-address-use-after-scope %s -o %t && %run %t
+
+// Test should not fail as access to address of my_char is done conditionally.
+
+int
+main (int argc, char **argv)
+{
+  char *ptr = 0;
+
+  if (argc == 123456789)
+    {
+      char my_char;
+      ptr = &my_char;
+    }
+
+  if (ptr)
+    return *ptr;
+
+  return 0;
+}