[PATCH] D14656: [sanitizer] Stop unwinding the stack when a close-to-zero PC is found

[Filipe Cabecinhas via llvm-commits]

On Mon, May 23, 2016 at 4:48 PM, Kuba Brecka <kuba.brecka at gmail.com> wrote:
> kubabrecka updated this revision to Diff 58101.
> kubabrecka added a comment.
> Herald added a subscriber: kubabrecka.
>
> Adding a unit test.  Changing GetPageSizeCached into an inlined function.
>
>
> http://reviews.llvm.org/D14656
>
> Files:
>   lib/sanitizer_common/sanitizer_common.cc
>   lib/sanitizer_common/sanitizer_common.h
>   lib/sanitizer_common/sanitizer_stacktrace.cc
>   lib/sanitizer_common/sanitizer_unwind_linux_libcdep.cc
>   lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc
>
> Index: lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc
> ===================================================================
> --- lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc
> +++ lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc
> @@ -136,6 +136,19 @@
>    EXPECT_EQ(PC(1), trace.trace[1]);
>  }
>
> +TEST_F(FastUnwindTest, CloseToZeroFrame) {
> +  // Make one pc a NULL pointer.
> +  fake_stack[5] = 0x0;
> +  if (!TryFastUnwind(kStackTraceMax))
> +    return;
> +  // The stack should be truncated at the NULL pointer (and not include it).
> +  EXPECT_EQ(3U, trace.size);
> +  EXPECT_EQ(start_pc, trace.trace[0]);
> +  for (uptr i = 1; i < 3U; i++) {
> +    EXPECT_EQ(PC(i*2 - 1), trace.trace[i]);
> +  }
> +}
> +
>  TEST(SlowUnwindTest, ShortStackTrace) {
>    if (StackTrace::WillUseFastUnwind(false))
>      return;
> Index: lib/sanitizer_common/sanitizer_unwind_linux_libcdep.cc
> ===================================================================
> --- lib/sanitizer_common/sanitizer_unwind_linux_libcdep.cc
> +++ lib/sanitizer_common/sanitizer_unwind_linux_libcdep.cc
> @@ -108,6 +108,8 @@
>    UnwindTraceArg *arg = (UnwindTraceArg*)param;
>    CHECK_LT(arg->stack->size, arg->max_depth);
>    uptr pc = Unwind_GetIP(ctx);
> +  const uptr kPageSize = GetPageSizeCached();
> +  if (pc < kPageSize) return UNWIND_STOP;
>    arg->stack->trace_buffer[arg->stack->size++] = pc;
>    if (arg->stack->size == arg->max_depth) return UNWIND_STOP;
>    return UNWIND_CONTINUE;
> Index: lib/sanitizer_common/sanitizer_stacktrace.cc
> ===================================================================
> --- lib/sanitizer_common/sanitizer_stacktrace.cc
> +++ lib/sanitizer_common/sanitizer_stacktrace.cc
> @@ -66,6 +66,7 @@
>
>  void BufferedStackTrace::FastUnwindStack(uptr pc, uptr bp, uptr stack_top,
>                                           uptr stack_bottom, u32 max_depth) {
> +  const uptr kPageSize = GetPageSizeCached();
>    CHECK_GE(max_depth, 2);
>    trace_buffer[0] = pc;
>    size = 1;
> @@ -92,6 +93,8 @@
>  #else
>      uhwptr pc1 = frame[1];
>  #endif
> +    if (pc1 < kPageSize)
> +      break;
>      if (pc1 != pc) {
>        trace_buffer[size++] = (uptr) pc1;
>      }
> Index: lib/sanitizer_common/sanitizer_common.h
> ===================================================================
> --- lib/sanitizer_common/sanitizer_common.h
> +++ lib/sanitizer_common/sanitizer_common.h
> @@ -63,7 +63,12 @@
>  }
>
>  uptr GetPageSize();
> -uptr GetPageSizeCached();
> +extern uptr PageSizeCached;
> +INLINE uptr GetPageSizeCached() {
> +  if (!PageSizeCached)
> +    PageSizeCached = GetPageSize();
> +  return PageSizeCached;
> +}

This will race and trigger UB.
Would it be possible to get the page size only on one or two places
when initializing?
That way, we'd only need an atomic load, and nothing else. We'd miss a
few places, where we could end up unwinding the stack, and
PageSizeCached is still 0, but those should be minor edge cases, no?

Thank you,

  Filipe

>  uptr GetMmapGranularity();
>  uptr GetMaxVirtualAddress();
>  // Threads
> Index: lib/sanitizer_common/sanitizer_common.cc
> ===================================================================
> --- lib/sanitizer_common/sanitizer_common.cc
> +++ lib/sanitizer_common/sanitizer_common.cc
> @@ -24,13 +24,7 @@
>  const char *SanitizerToolName = "SanitizerTool";
>
>  atomic_uint32_t current_verbosity;
> -
> -uptr GetPageSizeCached() {
> -  static uptr PageSize;
> -  if (!PageSize)
> -    PageSize = GetPageSize();
> -  return PageSize;
> -}
> +uptr PageSizeCached;
>
>  StaticSpinMutex report_file_mu;
>  ReportFile report_file = {&report_file_mu, kStderrFd, "", "", 0};
>
>