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

[Kuba Brecka via llvm-commits]

> On 25 May 2016, at 14:50, Filipe Cabecinhas <filcab+llvm.phabricator at gmail.com> wrote:
> 
> On Mon, May 23, 2016 at 4:48 PM, Kuba Brecka <kuba.brecka at gmail.com <mailto: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?

This doesn’t currently race, because the first call to this function happens when only one thread exists.  This behavior already exists and I’m not trying to change it in this patch.  I know this guarantee is weird and subtle, but I don’t think this is related to what I’m trying to achieve in this patch.

Kuba

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160525/5b610aa4/attachment.html>