[PATCH] Add -fsanitize=pointer-{compare,subtract}.
Martin Liška
mliska@suse.cz
Wed Oct 11 13:50:00 GMT 2017
On 10/11/2017 09:37 AM, Jakub Jelinek wrote:
> On Wed, Oct 11, 2017 at 07:55:44AM +0200, Martin Liška wrote:
>>> Conceptually, these two instrumentations rely on address sanitization,
>>> not really sure if we should supporting them for kernel sanitization (but I
>>> bet it is just going to be too costly for kernel).
>>> So, we also need to make sure at least parts of SANITIZE_ADDRESS is enabled
>>> when these options are on.
>>> That can be done by erroring out if -fsanitize=pointer-compare is requested
>>> without -fsanitize=address, or by implicitly enabling -fsanitize=address for
>>> these, or by adding yet another SANITIZE_* bit which would cover
>>> sanitization of memory accesses for asan, that bit would be set by
>>> -fsanitize={address,kernel-address} in addition to the current 2 bits, but
>>> pointer-{subtract,compare} would set its own bit and SANITIZE_ADDRESS and
>>> SANITIZE_USER_ADDRESS only. Without the new bit we'd emit red zones,
>>> function prologue/epilogue asan changes, registraction of global variables,
>>> but not actual instrumentation of memory accesses (and probably not
>>> instrumentation of C++ ctor ordering).
>>
>> Agree, would be much easier to just enable SANITIZE_ADDRESS with there 2 options.
>> Question is how to make it also possible with -fsanitize=kernel-address:
>>
>> $ ./xgcc -B. /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pointer-compare-2.c -fsanitize=pointer-compare,kernel-address
>> cc1: error: ‘-fsanitize=address’ is incompatible with ‘-fsanitize=kernel-address’
>>
>> Ideas?
>
Hello.
Thanks for feedback.
> If we want to make it usable for both user and kernel address, then either
> we'll let pointer-compare/pointer-subtract implicitly enable user address,
> unless kernel-address has been enabled (that would mean set SANITIZE_ADDRESS
> bit in pointer-*, but not SANITIZE_USER_ADDRESS, and at the point where we
> diagnose option incompatibilities like -fsanitize=address,kernel-address
> check for the case (SANITIZE_ADDRESS bit set, none of SANITIZE_USER_ADDRESS
> nor SANITIZE_KERNEL_ADDRESS, and one of SANITIZE_POINTER_*) and set
> implicitly SANITIZE_USER_ADDRESS, or simply require that the user chooses,
> by erroring out if pointer-* is used without explicit address or
> kernel-address. In any case, I think this should be also something
> discussed with the upstream sanitizer folks, so that LLVM (if it ever
> decides to actually implement it) behaves compatibly.
I've added support for automatic adding of -sanitize=address if none of them is added.
Problem is that LIBASAN_SPEC is still handled in driver. Thus I guess I'll need the hunks
I sent in first version of patch. Or do I miss something?
>
>> --- a/libsanitizer/asan/asan_descriptions.cc
>> +++ b/libsanitizer/asan/asan_descriptions.cc
>> @@ -220,6 +220,15 @@ bool GetStackAddressInformation(uptr addr, uptr access_size,
>> return true;
>> }
>>
>> +bool GetStackVariableBeginning(uptr addr, uptr *shadow_addr)
>> +{
>> + AsanThread *t = FindThreadByStackAddress(addr);
>> + if (!t) return false;
>> +
>> + *shadow_addr = t->GetStackFrameVariableBeginning (addr);
>> + return true;
>> +}
>> +
>> static void PrintAccessAndVarIntersection(const StackVarDescr &var, uptr addr,
>> uptr access_size, uptr prev_var_end,
>> uptr next_var_beg) {
>> diff --git a/libsanitizer/asan/asan_descriptions.h b/libsanitizer/asan/asan_descriptions.h
>> index 584b9ba6491..b7f23b1a71b 100644
>> --- a/libsanitizer/asan/asan_descriptions.h
>> +++ b/libsanitizer/asan/asan_descriptions.h
>> @@ -138,6 +138,7 @@ struct StackAddressDescription {
>>
>> bool GetStackAddressInformation(uptr addr, uptr access_size,
>> StackAddressDescription *descr);
>> +bool GetStackVariableBeginning(uptr addr, uptr *shadow_addr);
>>
>> struct GlobalAddressDescription {
>> uptr addr;
>> diff --git a/libsanitizer/asan/asan_globals.cc b/libsanitizer/asan/asan_globals.cc
>> index f2292926e6a..ed707c0ca01 100644
>> --- a/libsanitizer/asan/asan_globals.cc
>> +++ b/libsanitizer/asan/asan_globals.cc
>> @@ -122,6 +122,31 @@ int GetGlobalsForAddress(uptr addr, Global *globals, u32 *reg_sites,
>> return res;
>> }
>>
>> +bool AreGlobalVariablesSame(uptr addr1, uptr addr2)
>> +{
>> + if (addr1 > addr2)
>> + {
>> + uptr tmp = addr1;
>> + addr1 = addr2;
>> + addr2 = tmp;
>
> std::swap(addr1, addr2); ? I don't see it used in any of libsanitizer
> though, so not sure if the corresponding STL header is included.
They don't use it anywhere and I had some #include issues. That's why I did it manually.
>
>> + }
>> +
>> + BlockingMutexLock lock(&mu_for_globals);
>
> Why do you need a mutex for checking if there are no red zones in between?
>
>> + uptr aligned_addr1 = addr1 & ~(SANITIZER_WORDSIZE/8 - 1); // align addr.
>> + uptr aligned_addr2 = addr2 & ~(SANITIZER_WORDSIZE/8 - 1); // align addr.
>> +
>> + u8 *shadow_ptr1 = (u8*)MemToShadow(aligned_addr1);
>> + u8 *shadow_ptr2 = (u8*)MemToShadow(aligned_addr2);
>> +
>> + while (shadow_ptr1 <= shadow_ptr2
>> + && *shadow_ptr1 != kAsanGlobalRedzoneMagic) {
>> + shadow_ptr1++;
>> + }
>
> There are many kinds of shadow memory markings. My thought was that it
> would start with a quick check, perhaps vectorized by hand (depending on if
> the arch has unaligned loads maybe without or with a short loop for
Did that, but I have no experience how to make decision about prologue that will
align the pointer? Any examples?
> alignment) where say unsigned long (perhaps may_alias?) pointer would be
> used to read 4/8 shadow bytes at a time, and just check if any of them is
> non-zero. And, if it found a non-zero byte, deal with it after the loop,
> if after the "vectorized" loop, find which byte it was, and in any case
> deal e.g. with the various special cases like when the shadow byte is 1-7
> and stands for how many bytes are accessible, etc.
Yes, can help significantly I guess.
Martin
>
>> --- a/libsanitizer/asan/asan_report.cc
>> +++ b/libsanitizer/asan/asan_report.cc
>> @@ -344,14 +344,52 @@ static INLINE void CheckForInvalidPointerPair(void *p1, void *p2) {
>> if (!flags()->detect_invalid_pointer_pairs) return;
>> uptr a1 = reinterpret_cast<uptr>(p1);
>> uptr a2 = reinterpret_cast<uptr>(p2);
>> - AsanChunkView chunk1 = FindHeapChunkByAddress(a1);
>> - AsanChunkView chunk2 = FindHeapChunkByAddress(a2);
>> - bool valid1 = chunk1.IsAllocated();
>> - bool valid2 = chunk2.IsAllocated();
>> - if (!valid1 || !valid2 || !chunk1.Eq(chunk2)) {
>> - GET_CALLER_PC_BP_SP;
>> - return ReportInvalidPointerPair(pc, bp, sp, a1, a2);
>> +
>> + if (a1 == a2)
>> + return;
>
> My thought was that you'd do the difference <= 2048 test first
> without any lock, and only for the larger stuff take locks and look stuff
> up.
>
>> +
>> + uptr shadow_offset1, shadow_offset2;
>> + bool valid1, valid2;
>> + {
>> + ThreadRegistryLock l(&asanThreadRegistry());
>> +
>> + valid1 = GetStackVariableBeginning(a1, &shadow_offset1);
>> + valid2 = GetStackVariableBeginning(a2, &shadow_offset2);
>> + }
>> +
>> + if (valid1 && valid2) {
>> + if (shadow_offset1 == shadow_offset2)
>> + return;
>
>
>> }
>> + else if (!valid1 && !valid2) {
>> + AsanChunkView chunk1 = FindHeapChunkByAddress(a1);
>> + AsanChunkView chunk2 = FindHeapChunkByAddress(a2);
>> + valid1 = chunk1.IsAllocated();
>> + valid2 = chunk2.IsAllocated();
>> +
>> + if (valid1 && valid2) {
>> + if (chunk1.Eq(chunk2))
>> + return;
>> + }
>> + else if (!valid1 && !valid2) {
>> + uptr offset = a1 < a2 ? a2 - a1 : a1 - a2;
>> + if (offset <= 2048) {
>> + if (AreGlobalVariablesSame (a1, a2))
>> + return;
>> + }
>> +
>> + GlobalAddressDescription gdesc1, gdesc2;
>> + valid1 = GetGlobalAddressInformation(a1, 1, &gdesc1);
>> + valid2 = GetGlobalAddressInformation(a2, 1, &gdesc2);
>> +
>> + if (valid1 && valid2
>> + && gdesc1.globals[0].beg == gdesc2.globals[0].beg)
>> + return;
>> + }
>> + }
>> +
>> + GET_CALLER_PC_BP_SP;
>> + ReportInvalidPointerPair(pc, bp, sp, a1, a2);
>> }
>> // ----------------------- Mac-specific reports ----------------- {{{1
>>
>> diff --git a/libsanitizer/asan/asan_report.h b/libsanitizer/asan/asan_report.h
>> index 111b8400153..2b4c9d29fda 100644
>> --- a/libsanitizer/asan/asan_report.h
>> +++ b/libsanitizer/asan/asan_report.h
>> @@ -27,6 +27,7 @@ struct StackVarDescr {
>> // them to "globals" array.
>> int GetGlobalsForAddress(uptr addr, __asan_global *globals, u32 *reg_sites,
>> int max_globals);
>> +bool AreGlobalVariablesSame(uptr addr1, uptr addr2);
>>
>> const char *MaybeDemangleGlobalName(const char *name);
>> void PrintGlobalNameIfASCII(InternalScopedString *str, const __asan_global &g);
>> diff --git a/libsanitizer/asan/asan_thread.cc b/libsanitizer/asan/asan_thread.cc
>> index 818e1261400..d6bd051a493 100644
>> --- a/libsanitizer/asan/asan_thread.cc
>> +++ b/libsanitizer/asan/asan_thread.cc
>> @@ -322,6 +322,29 @@ bool AsanThread::GetStackFrameAccessByAddr(uptr addr,
>> return true;
>> }
>>
>> +uptr AsanThread::GetStackFrameVariableBeginning(uptr addr)
>> +{
>> + uptr bottom = 0;
>> + if (AddrIsInStack(addr)) {
>> + bottom = stack_bottom();
>> + } else if (has_fake_stack()) {
>> + bottom = fake_stack()->AddrIsInFakeStack(addr);
>> + CHECK(bottom);
>> + }
>> + uptr aligned_addr = addr & ~(SANITIZER_WORDSIZE/8 - 1); // align addr.
>> + u8 *shadow_ptr = (u8*)MemToShadow(aligned_addr);
>> + u8 *shadow_bottom = (u8*)MemToShadow(bottom);
>> +
>> + while (shadow_ptr >= shadow_bottom &&
>> + (*shadow_ptr != kAsanStackLeftRedzoneMagic
>> + && *shadow_ptr != kAsanStackMidRedzoneMagic
>> + && *shadow_ptr != kAsanStackRightRedzoneMagic)) {
>> + shadow_ptr--;
>> + }
>> +
>> + return (uptr)shadow_ptr;
>> +}
>> +
>> bool AsanThread::AddrIsInStack(uptr addr) {
>> const auto bounds = GetStackBounds();
>> return addr >= bounds.bottom && addr < bounds.top;
>> diff --git a/libsanitizer/asan/asan_thread.h b/libsanitizer/asan/asan_thread.h
>> index c51a58ad0bb..c5adecacad4 100644
>> --- a/libsanitizer/asan/asan_thread.h
>> +++ b/libsanitizer/asan/asan_thread.h
>> @@ -80,6 +80,7 @@ class AsanThread {
>> const char *frame_descr;
>> };
>> bool GetStackFrameAccessByAddr(uptr addr, StackFrameAccess *access);
>> + uptr GetStackFrameVariableBeginning(uptr addr);
>>
>> bool AddrIsInStack(uptr addr);
>>
>> --
>> 2.14.2
>>
>
>
> Jakub
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-fsanitize-pointer-compare-subtract-v3.patch
Type: text/x-patch
Size: 21854 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20171011/45ebc6f8/attachment.bin>
More information about the Gcc-patches
mailing list