Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On 12/05/2017 10:27 AM, Jakub Jelinek wrote: > The most important change I've done in the testsuite was pointer-subtract-2.c > used -fsanitize=address,pointer-subtract, but the function was actually > doing pointer comparison. Guess that needs to be propagated upstream at > some point. Another thing is that in pointer-compare-1.c where you test > p - 1, p and p, p - 1 on the global variables, we need to ensure there is > some other array before it, otherwise we run into the issue that there is no > red zone before the first global (and when optimizing, global objects seems > to be sorted by decreasing size). Hi. I've just done review request for that: https://reviews.llvm.org/D41481 Apart from that I enhanced detect_invalid_pointer_pairs run-time option that can control whether a pointer comparison (or subtraction) with nullptr is reported or not: https://reviews.llvm.org/D41479 Martin
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On 12/05/2017 10:27 AM, Jakub Jelinek wrote: > On Tue, Nov 21, 2017 at 12:59:46PM +0100, Martin Liška wrote: >> On 10/16/2017 10:39 PM, Martin Liška wrote: >>> Hi. >>> >>> All nits included in mainline review request I've just done: >>> https://reviews.llvm.org/D38971 >>> >>> Martin >> >> Hi. >> >> There's updated version of patch where I added new test-cases and it's >> rebased >> with latest version of libsanitizer changes. This is subject for >> libsanitizer review process. > > A slightly modified version has been finally accepted upstream, here is the > updated patch with that final version cherry-picked, plus small changes to > make it apply after the POINTER_DIFF_EXPR changes, and a lot of testsuite > tweaks, so that we don't run it 7 times with -O0, but with different > optimization levels, cleanups etc. Hi Jakub. Thanks for finalizing the patch review and for the clean up you've done. > The most important change I've done in the testsuite was pointer-subtract-2.c > used -fsanitize=address,pointer-subtract, but the function was actually > doing pointer comparison. Guess that needs to be propagated upstream at > some point. Another thing is that in pointer-compare-1.c where you test > p - 1, p and p, p - 1 on the global variables, we need to ensure there is > some other array before it, otherwise we run into the issue that there is no > red zone before the first global (and when optimizing, global objects seems > to be sorted by decreasing size). I'll add there minor issues to my TODO list and I'll create mainline patch review. Martin > > Bootstrapped/regtested on x86_64-linux and i686-linux, committed to trunk. > > 2017-12-05 Martin Liska> Jakub Jelinek > > gcc/ > * doc/invoke.texi: Document the options. > * flag-types.h (enum sanitize_code): Add > SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. > * ipa-inline.c (sanitize_attrs_match_for_inline_p): Add handling > of SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. > * opts.c: Define new sanitizer options. > * sanitizer.def (BUILT_IN_ASAN_POINTER_COMPARE): Likewise. > (BUILT_IN_ASAN_POINTER_SUBTRACT): Likewise. > gcc/c/ > * c-typeck.c (pointer_diff): Add new argument and instrument > pointer subtraction. > (build_binary_op): Similar for pointer comparison. > gcc/cp/ > * typeck.c (pointer_diff): Add new argument and instrument > pointer subtraction. > (cp_build_binary_op): Create compound expression if doing an > instrumentation. > gcc/testsuite/ > * c-c++-common/asan/pointer-compare-1.c: New test. > * c-c++-common/asan/pointer-compare-2.c: New test. > * c-c++-common/asan/pointer-subtract-1.c: New test. > * c-c++-common/asan/pointer-subtract-2.c: New test. > * c-c++-common/asan/pointer-subtract-3.c: New test. > * c-c++-common/asan/pointer-subtract-4.c: New test. > libsanitizer/ > * asan/asan_descriptions.cc: Cherry-pick upstream r319668. > * asan/asan_descriptions.h: Likewise. > * asan/asan_report.cc: Likewise. > * asan/asan_thread.cc: Likewise. > * asan/asan_thread.h: Likewise. > > --- gcc/c/c-typeck.c.jj 2017-12-01 19:42:09.504222186 +0100 > +++ gcc/c/c-typeck.c 2017-12-04 22:41:50.680290866 +0100 > @@ -95,7 +95,7 @@ static tree lookup_field (tree, tree); > static int convert_arguments (location_t, vec, tree, > vec *, vec *, tree, > tree); > -static tree pointer_diff (location_t, tree, tree); > +static tree pointer_diff (location_t, tree, tree, tree *); > static tree convert_for_assignment (location_t, location_t, tree, tree, tree, > enum impl_conv, bool, tree, tree, int); > static tree valid_compound_expr_initializer (tree, tree); > @@ -3768,10 +3768,11 @@ parser_build_binary_op (location_t locat > } > > /* Return a tree for the difference of pointers OP0 and OP1. > - The resulting tree has type ptrdiff_t. */ > + The resulting tree has type ptrdiff_t. If POINTER_SUBTRACT sanitization > is > + enabled, assign to INSTRUMENT_EXPR call to libsanitizer. */ > > static tree > -pointer_diff (location_t loc, tree op0, tree op1) > +pointer_diff (location_t loc, tree op0, tree op1, tree *instrument_expr) > { >tree restype = ptrdiff_type_node; >tree result, inttype; > @@ -3815,6 +3816,17 @@ pointer_diff (location_t loc, tree op0, > pedwarn (loc, OPT_Wpointer_arith, >"pointer to a function used in subtraction"); > > + if (sanitize_flags_p (SANITIZE_POINTER_SUBTRACT)) > +{ > + gcc_assert (current_function_decl != NULL_TREE); > + > + op0 = save_expr (op0); > + op1 = save_expr (op1); > + > + tree tt = builtin_decl_explicit (BUILT_IN_ASAN_POINTER_SUBTRACT); > + *instrument_expr = build_call_expr_loc (loc, tt, 2, op0, op1); > +} >
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On Tue, Nov 21, 2017 at 12:59:46PM +0100, Martin Liška wrote: > On 10/16/2017 10:39 PM, Martin Liška wrote: > > Hi. > > > > All nits included in mainline review request I've just done: > > https://reviews.llvm.org/D38971 > > > > Martin > > Hi. > > There's updated version of patch where I added new test-cases and it's rebased > with latest version of libsanitizer changes. This is subject for libsanitizer > review process. A slightly modified version has been finally accepted upstream, here is the updated patch with that final version cherry-picked, plus small changes to make it apply after the POINTER_DIFF_EXPR changes, and a lot of testsuite tweaks, so that we don't run it 7 times with -O0, but with different optimization levels, cleanups etc. The most important change I've done in the testsuite was pointer-subtract-2.c used -fsanitize=address,pointer-subtract, but the function was actually doing pointer comparison. Guess that needs to be propagated upstream at some point. Another thing is that in pointer-compare-1.c where you test p - 1, p and p, p - 1 on the global variables, we need to ensure there is some other array before it, otherwise we run into the issue that there is no red zone before the first global (and when optimizing, global objects seems to be sorted by decreasing size). Bootstrapped/regtested on x86_64-linux and i686-linux, committed to trunk. 2017-12-05 Martin LiskaJakub Jelinek gcc/ * doc/invoke.texi: Document the options. * flag-types.h (enum sanitize_code): Add SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. * ipa-inline.c (sanitize_attrs_match_for_inline_p): Add handling of SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. * opts.c: Define new sanitizer options. * sanitizer.def (BUILT_IN_ASAN_POINTER_COMPARE): Likewise. (BUILT_IN_ASAN_POINTER_SUBTRACT): Likewise. gcc/c/ * c-typeck.c (pointer_diff): Add new argument and instrument pointer subtraction. (build_binary_op): Similar for pointer comparison. gcc/cp/ * typeck.c (pointer_diff): Add new argument and instrument pointer subtraction. (cp_build_binary_op): Create compound expression if doing an instrumentation. gcc/testsuite/ * c-c++-common/asan/pointer-compare-1.c: New test. * c-c++-common/asan/pointer-compare-2.c: New test. * c-c++-common/asan/pointer-subtract-1.c: New test. * c-c++-common/asan/pointer-subtract-2.c: New test. * c-c++-common/asan/pointer-subtract-3.c: New test. * c-c++-common/asan/pointer-subtract-4.c: New test. libsanitizer/ * asan/asan_descriptions.cc: Cherry-pick upstream r319668. * asan/asan_descriptions.h: Likewise. * asan/asan_report.cc: Likewise. * asan/asan_thread.cc: Likewise. * asan/asan_thread.h: Likewise. --- gcc/c/c-typeck.c.jj 2017-12-01 19:42:09.504222186 +0100 +++ gcc/c/c-typeck.c2017-12-04 22:41:50.680290866 +0100 @@ -95,7 +95,7 @@ static tree lookup_field (tree, tree); static int convert_arguments (location_t, vec, tree, vec *, vec *, tree, tree); -static tree pointer_diff (location_t, tree, tree); +static tree pointer_diff (location_t, tree, tree, tree *); static tree convert_for_assignment (location_t, location_t, tree, tree, tree, enum impl_conv, bool, tree, tree, int); static tree valid_compound_expr_initializer (tree, tree); @@ -3768,10 +3768,11 @@ parser_build_binary_op (location_t locat } /* Return a tree for the difference of pointers OP0 and OP1. - The resulting tree has type ptrdiff_t. */ + The resulting tree has type ptrdiff_t. If POINTER_SUBTRACT sanitization is + enabled, assign to INSTRUMENT_EXPR call to libsanitizer. */ static tree -pointer_diff (location_t loc, tree op0, tree op1) +pointer_diff (location_t loc, tree op0, tree op1, tree *instrument_expr) { tree restype = ptrdiff_type_node; tree result, inttype; @@ -3815,6 +3816,17 @@ pointer_diff (location_t loc, tree op0, pedwarn (loc, OPT_Wpointer_arith, "pointer to a function used in subtraction"); + if (sanitize_flags_p (SANITIZE_POINTER_SUBTRACT)) +{ + gcc_assert (current_function_decl != NULL_TREE); + + op0 = save_expr (op0); + op1 = save_expr (op1); + + tree tt = builtin_decl_explicit (BUILT_IN_ASAN_POINTER_SUBTRACT); + *instrument_expr = build_call_expr_loc (loc, tt, 2, op0, op1); +} + /* First do the subtraction, then build the divide operator and only convert at the very end. Do not do default conversions in case restype is a short type. */ @@ -3825,8 +3837,8 @@ pointer_diff (location_t loc, tree op0, space, cast the pointers to some larger integer type and do the computations in that type. */ if
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On 10/16/2017 10:39 PM, Martin Liška wrote: > Hi. > > All nits included in mainline review request I've just done: > https://reviews.llvm.org/D38971 > > Martin Hi. There's updated version of patch where I added new test-cases and it's rebased with latest version of libsanitizer changes. This is subject for libsanitizer review process. Martin >From 100b723b9b7fb10dedb2154f30e1ebd6ef885ab4 Mon Sep 17 00:00:00 2001 From: marxinDate: Wed, 8 Nov 2017 13:16:17 +0100 Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}. gcc/ChangeLog: 2017-11-21 Martin Liska * doc/invoke.texi: Document the options. * flag-types.h (enum sanitize_code): Add SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. * ipa-inline.c (sanitize_attrs_match_for_inline_p): Add handling of SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. * opts.c: Define new sanitizer options. * sanitizer.def (BUILT_IN_ASAN_POINTER_COMPARE): Likewise. (BUILT_IN_ASAN_POINTER_SUBTRACT): Likewise. gcc/c/ChangeLog: 2017-11-21 Martin Liska * c-typeck.c (pointer_diff): Add new argument and instrument pointer subtraction. (build_binary_op): Similar for pointer comparison. gcc/cp/ChangeLog: 2017-11-21 Martin Liska * typeck.c (pointer_diff): Add new argument and instrument pointer subtraction. (cp_build_binary_op): Create compound expression if doing an instrumentation. gcc/testsuite/ChangeLog: 2017-11-21 Martin Liska * c-c++-common/asan/pointer-compare-1.c: New test. * c-c++-common/asan/pointer-compare-2.c: New test. * c-c++-common/asan/pointer-subtract-1.c: New test. * c-c++-common/asan/pointer-subtract-2.c: New test. * c-c++-common/asan/pointer-subtract-3.c: New test. * c-c++-common/asan/pointer-subtract-4.c: New test. --- gcc/c/c-typeck.c | 31 ++-- gcc/cp/typeck.c| 39 -- gcc/doc/invoke.texi| 22 ++ gcc/flag-types.h | 2 + gcc/ipa-inline.c | 8 ++- gcc/opts.c | 15 gcc/sanitizer.def | 4 ++ .../c-c++-common/asan/pointer-compare-1.c | 83 ++ .../c-c++-common/asan/pointer-compare-2.c | 76 .../c-c++-common/asan/pointer-subtract-1.c | 41 +++ .../c-c++-common/asan/pointer-subtract-2.c | 33 + .../c-c++-common/asan/pointer-subtract-3.c | 40 +++ .../c-c++-common/asan/pointer-subtract-4.c | 40 +++ libsanitizer/asan/asan_descriptions.cc | 20 ++ libsanitizer/asan/asan_descriptions.h | 4 ++ libsanitizer/asan/asan_report.cc | 53 -- libsanitizer/asan/asan_thread.cc | 25 ++- libsanitizer/asan/asan_thread.h| 3 + 18 files changed, 521 insertions(+), 18 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/asan/pointer-compare-1.c create mode 100644 gcc/testsuite/c-c++-common/asan/pointer-compare-2.c create mode 100644 gcc/testsuite/c-c++-common/asan/pointer-subtract-1.c create mode 100644 gcc/testsuite/c-c++-common/asan/pointer-subtract-2.c create mode 100644 gcc/testsuite/c-c++-common/asan/pointer-subtract-3.c create mode 100644 gcc/testsuite/c-c++-common/asan/pointer-subtract-4.c diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 4bdc48a9ea3..5dac9bdf08b 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -96,7 +96,7 @@ static tree lookup_field (tree, tree); static int convert_arguments (location_t, vec, tree, vec *, vec *, tree, tree); -static tree pointer_diff (location_t, tree, tree); +static tree pointer_diff (location_t, tree, tree, tree *); static tree convert_for_assignment (location_t, location_t, tree, tree, tree, enum impl_conv, bool, tree, tree, int); static tree valid_compound_expr_initializer (tree, tree); @@ -3778,10 +3778,11 @@ parser_build_binary_op (location_t location, enum tree_code code, } /* Return a tree for the difference of pointers OP0 and OP1. - The resulting tree has type int. */ + The resulting tree has type int. If POINTER_SUBTRACT sanitization is + enabled, assign to INSTRUMENT_EXPR call to libsanitizer. */ static tree -pointer_diff (location_t loc, tree op0, tree op1) +pointer_diff (location_t loc, tree op0, tree op1, tree *instrument_expr) { tree restype = ptrdiff_type_node; tree result, inttype; @@ -3825,6 +3826,17 @@ pointer_diff (location_t loc, tree op0, tree op1) pedwarn (loc, OPT_Wpointer_arith, "pointer to a function used in subtraction"); + if (sanitize_flags_p (SANITIZE_POINTER_SUBTRACT)) +{ + gcc_assert (current_function_decl != NULL_TREE); + + op0 = save_expr (op0); +
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
Hi. All nits included in mainline review request I've just done: https://reviews.llvm.org/D38971 Martin
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On Mon, Oct 16, 2017 at 03:38:28PM +0200, Martin Liška wrote: > --- a/libsanitizer/asan/asan_report.cc > +++ b/libsanitizer/asan/asan_report.cc > @@ -344,14 +344,70 @@ static INLINE void CheckForInvalidPointerPair(void *p1, > void *p2) { >if (!flags()->detect_invalid_pointer_pairs) return; >uptr a1 = reinterpret_cast(p1); >uptr a2 = reinterpret_cast(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; > + > + uptr offset = a1 < a2 ? a2 - a1 : a1 - a2; > + uptr left = a1 < a2 ? a1 : a2; > + uptr right = a1 < a2 ? a2 : a1; > + if (offset <= 2048) { > +if (__asan_region_is_poisoned(left, offset) == 0) > + return; > +else > + goto do_error; > + } > + > + { > +uptr shadow_offset1, shadow_offset2; Some more nits. This is C++ (but C99 would do as well), you don't need a new scope for the above variables. Just declare them without {} around and extra indentation. > + > +{ > + ThreadRegistryLock l(()); > + > + // check whether left is a stack memory pointer > + if (GetStackVariableBeginning(left, _offset1)) { > + if (GetStackVariableBeginning(right - 1, _offset2) && > + shadow_offset1 == shadow_offset2) > + return; > + else > + goto do_error; > + } > +} > + > +// check whether left is a heap memory address > +HeapAddressDescription hdesc1, hdesc2; > +if (GetHeapAddressInformation(left, 0, ) && > + hdesc1.chunk_access.access_type == kAccessTypeInside) { > + if (GetHeapAddressInformation(right, 0, ) && > + hdesc2.chunk_access.access_type == kAccessTypeInside && > + (hdesc1.chunk_access.chunk_begin > +== hdesc2.chunk_access.chunk_begin)) > + return; > + else > + goto do_error; > +} > +// check whether left is an address of a global variable > +GlobalAddressDescription gdesc1, gdesc2; > +if (GetGlobalAddressInformation(left, 0, )) { > + if (GetGlobalAddressInformation(right - 1, 0, ) && > + gdesc1.PointsInsideASameVariable (gdesc2)) > + return; I think it is better to use consistent coding, i.e. use else goto do_error; here too. > +} > +else { Without the else { here. And do { ThreadRegistryLock l(()); if (GetStackVariableBeginning(right - 1, _offset2)) goto do_error; } if (GetHeapAddressInformation(right, 0, ) || GetGlobalAddressInformation(right - 1, 0, )) goto do_error; /* At this point we know nothing about both a1 and a2 addresses. */ return; so that the lock is held over GetStackVariableBeginning only. Jakub
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On 10/16/2017 02:21 PM, Jakub Jelinek wrote: > On Mon, Oct 16, 2017 at 01:57:59PM +0200, Martin Liška wrote: >> Agree. Do you feel that it's right moment to trigger review process of >> libsanitizer >> changes? > > Yes. We don't have that much time left to get it through... Good, I've triggered regression tests. Will send it to phsabricator later this evening. > >> --- a/libsanitizer/asan/asan_report.cc >> +++ b/libsanitizer/asan/asan_report.cc >> + { >> +uptr shadow_offset1, shadow_offset2; >> +ThreadRegistryLock l(()); >> + >> +// check whether left is a stack memory pointer >> +if (GetStackVariableBeginning(left, _offset1)) { >> + if (GetStackVariableBeginning(right - 1, _offset2) && >> + shadow_offset1 == shadow_offset2) >> +return; >> + else >> +goto do_error; >> +} > > Do you need the ThreadRegistryLock for the following calls? > If not, the } should be closed and it should be reindented. Yes, we do. > >> +// check whether left is a heap memory address >> +HeapAddressDescription hdesc1, hdesc2; >> +if (GetHeapAddressInformation(left, 0, ) && >> +hdesc1.chunk_access.access_type == kAccessTypeInside) { >> + if (GetHeapAddressInformation(right, 0, ) && >> + hdesc2.chunk_access.access_type == kAccessTypeInside && >> + (hdesc1.chunk_access.chunk_begin >> + == hdesc2.chunk_access.chunk_begin)) >> +return; >> + else >> +goto do_error; >> +} >> +// check whether left is an address of a global variable >> +GlobalAddressDescription gdesc1, gdesc2; >> +if (GetGlobalAddressInformation(left, 0, )) { >> + if (GetGlobalAddressInformation(right - 1, 0, ) && >> + gdesc1.PointsInsideASameVariable (gdesc2)) >> +return; >> +} else >> + goto do_error; > > ?? If we don't know anything about the left object, doing a goto do_error; > sounds dangerous, it could be say mmapped region or whatever else. Good point! > > Though, we could at that spot at least check if right isn't one > of the 3 kinds of regions we track and if yes, error out. > So perhaps move the else goto do_error; inside of the {} and > do > if (GetStackVariableBeginning(right - 1, _offset2) || > GetHeapAddressInformation(right, 0, ) || > GetGlobalAddressInformation(right - 1, 0, )) > goto do_error; > return; > (if the lock above is released, you'd of course need to retake it for > if (GetStackVariableBeginning(right - 1, _offset2)). Yes, should be fixed now. Martin > > Jakub > >From 55e226413b9b3533b4167a4895b40f66cd665f11 Mon Sep 17 00:00:00 2001 From: marxinDate: Thu, 5 Oct 2017 12:14:25 +0200 Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}. gcc/ChangeLog: 2017-10-13 Martin Liska * doc/invoke.texi: Document the options. * flag-types.h (enum sanitize_code): Add SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. * ipa-inline.c (sanitize_attrs_match_for_inline_p): Add handling of SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. * opts.c: Define new sanitizer options. * sanitizer.def (BUILT_IN_ASAN_POINTER_COMPARE): Likewise. (BUILT_IN_ASAN_POINTER_SUBTRACT): Likewise. gcc/c/ChangeLog: 2017-10-13 Martin Liska * c-typeck.c (pointer_diff): Add new argument and instrument pointer subtraction. (build_binary_op): Similar for pointer comparison. gcc/cp/ChangeLog: 2017-10-16 Martin Liska * typeck.c (pointer_diff): Add new argument and instrument pointer subtraction. (cp_build_binary_op): Create compound expression if doing an instrumentation. gcc/testsuite/ChangeLog: 2017-10-16 Martin Liska * c-c++-common/asan/pointer-compare-1.c: New test. * c-c++-common/asan/pointer-compare-2.c: New test. * c-c++-common/asan/pointer-subtract-1.c: New test. * c-c++-common/asan/pointer-subtract-2.c: New test. --- gcc/c/c-typeck.c | 33 +++-- gcc/cp/typeck.c| 43 +-- gcc/doc/invoke.texi| 22 ++ gcc/flag-types.h | 2 + gcc/ipa-inline.c | 8 ++- gcc/opts.c | 15 gcc/sanitizer.def | 4 ++ .../c-c++-common/asan/pointer-compare-1.c | 83 ++ .../c-c++-common/asan/pointer-compare-2.c | 76 .../c-c++-common/asan/pointer-subtract-1.c | 41 +++ .../c-c++-common/asan/pointer-subtract-2.c | 32 + libsanitizer/asan/asan_descriptions.cc | 29 libsanitizer/asan/asan_descriptions.h | 5 ++ libsanitizer/asan/asan_report.cc | 70 -- libsanitizer/asan/asan_thread.cc | 23 ++ libsanitizer/asan/asan_thread.h| 3 + 16
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On Mon, Oct 16, 2017 at 01:57:59PM +0200, Martin Liška wrote: > Agree. Do you feel that it's right moment to trigger review process of > libsanitizer > changes? Yes. We don't have that much time left to get it through... > --- a/libsanitizer/asan/asan_report.cc > +++ b/libsanitizer/asan/asan_report.cc > + { > +uptr shadow_offset1, shadow_offset2; > +ThreadRegistryLock l(()); > + > +// check whether left is a stack memory pointer > +if (GetStackVariableBeginning(left, _offset1)) { > + if (GetStackVariableBeginning(right - 1, _offset2) && > + shadow_offset1 == shadow_offset2) > + return; > + else > + goto do_error; > +} Do you need the ThreadRegistryLock for the following calls? If not, the } should be closed and it should be reindented. > +// check whether left is a heap memory address > +HeapAddressDescription hdesc1, hdesc2; > +if (GetHeapAddressInformation(left, 0, ) && > + hdesc1.chunk_access.access_type == kAccessTypeInside) { > + if (GetHeapAddressInformation(right, 0, ) && > + hdesc2.chunk_access.access_type == kAccessTypeInside && > + (hdesc1.chunk_access.chunk_begin > +== hdesc2.chunk_access.chunk_begin)) > + return; > + else > + goto do_error; > +} > +// check whether left is an address of a global variable > +GlobalAddressDescription gdesc1, gdesc2; > +if (GetGlobalAddressInformation(left, 0, )) { > + if (GetGlobalAddressInformation(right - 1, 0, ) && > + gdesc1.PointsInsideASameVariable (gdesc2)) > + return; > +} else > + goto do_error; ?? If we don't know anything about the left object, doing a goto do_error; sounds dangerous, it could be say mmapped region or whatever else. Though, we could at that spot at least check if right isn't one of the 3 kinds of regions we track and if yes, error out. So perhaps move the else goto do_error; inside of the {} and do if (GetStackVariableBeginning(right - 1, _offset2) || GetHeapAddressInformation(right, 0, ) || GetGlobalAddressInformation(right - 1, 0, )) goto do_error; return; (if the lock above is released, you'd of course need to retake it for if (GetStackVariableBeginning(right - 1, _offset2)). Jakub
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On 10/13/2017 03:13 PM, Jakub Jelinek wrote: > On Fri, Oct 13, 2017 at 02:53:50PM +0200, Martin Liška wrote: >> @@ -3826,6 +3827,19 @@ pointer_diff (location_t loc, tree op0, tree op1) >> pedwarn (loc, OPT_Wpointer_arith, >> "pointer to a function used in subtraction"); >> >> + if (sanitize_flags_p (SANITIZE_POINTER_SUBTRACT)) >> +{ >> + gcc_assert (current_function_decl != NULL_TREE); >> + >> + op0 = save_expr (op0); >> + op1 = save_expr (op1); >> + >> + tree tt = builtin_decl_explicit (BUILT_IN_ASAN_POINTER_SUBTRACT); >> + *instrument_expr >> += build_call_expr_loc (loc, tt, 2, c_fully_fold (op0, false, NULL), >> + c_fully_fold (op1, false, NULL)); >> +} > Hello Jakub. > Why the c_fully_fold? Can't that be deferred until it actually is > folded all together later? Yes, now it's not needed as I use save_expr. I hit some ICE before I switched to use save_expr. That's why I put it there. > >> + ret = pointer_diff (location, op0, op1, _expr); >>goto return_build_binary_op; >> } >>/* Handle pointer minus int. Just like pointer plus int. */ >> @@ -11707,6 +11721,18 @@ build_binary_op (location_t location, enum >> tree_code code, >>pedwarn (location, 0, >> "comparison of distinct pointer types lacks a cast"); >> } >> + >> + if (sanitize_flags_p (SANITIZE_POINTER_COMPARE)) >> +{ >> + gcc_assert (current_function_decl != NULL_TREE); >> + >> + op0 = save_expr (op0); >> + op1 = save_expr (op1); >> + >> + tree tt = builtin_decl_explicit (BUILT_IN_ASAN_POINTER_COMPARE); >> + instrument_expr >> += build_call_expr_loc (location, tt, 2, op0, op1); >> +} > > Is this the right spot for this? I mean then you don't handle > ptr >= 0 or ptr > 0 and similar or ptr >= 0x12345678. > I know we have warnings or pedwarns for those, still I think it would be > better to handle the above e.g. before > if ((TREE_CODE (TREE_TYPE (orig_op0)) == BOOLEAN_TYPE >|| truth_value_p (TREE_CODE (orig_op0))) > ^ (TREE_CODE (TREE_TYPE (orig_op1)) == BOOLEAN_TYPE > || truth_value_p (TREE_CODE (orig_op1 > maybe_warn_bool_compare (location, code, orig_op0, orig_op1); > by testing if ((code0 == POINTER_TYPE || code1 == POINTER_TYPE) > && sanitize_flags_p (SANITIZE_POINTER_COMPARE)) Agree. > > What about the C++ FE? Or is pointer comparison well defined there? > What about pointer subtraction? My memory is fuzzy. No, you're right, I basically forgot: ``` >From § 5.9 of the C++11 standard: If two pointers p and q of the same type point to different objects that are not members of the same object or elements of the same array or to different functions, or if only one of them is null, the results of pq, p<=q, and p>=q are unspecified. ``` I also moved the tests to c-c++-common/asan/ subfolder. > >> +// { dg-options "-fsanitize=pointer-compare -O0" } > > Please use -fsanitize=address,pointer-compare > etc. in the testcases, so that it is an example to users who don't know > we have implicit -fsanitize=address for these tests. >> + if (offset <= 2048) { >> +if (__asan_region_is_poisoned (left, offset) == 0) > > I think the LLVM coding conventions don't want a space before ( above. > >> +// check whether left is a stack memory pointer >> +if (GetStackVariableBeginning(left, _offset1)) { >> + if (GetStackVariableBeginning(right - 1, _offset2) >> + && shadow_offset1 == shadow_offset2) > > Nor && at the beginning of the line (they want it at the end of previous > one). > >> +return; >> + else >> +goto do_error; >> +} > > If you have goto do_error; for all cases, then you don't need to indent > further stuff into else ... further and further. > >> +// check whether left is a heap memory address >> +else { >> + HeapAddressDescription hdesc1, hdesc2; >> + if (GetHeapAddressInformation(left, 0, ) >> + && hdesc1.chunk_access.access_type == kAccessTypeInside) { >> +if (GetHeapAddressInformation(right, 0, ) >> +&& hdesc2.chunk_access.access_type == kAccessTypeInside >> +&& (hdesc1.chunk_access.chunk_begin >> + == hdesc2.chunk_access.chunk_begin)) >> + return; > > So, here one is a heap object and the other is not. That should be an > do_error, right? Yes, coding style should be fixed. > >> + } else { >> +// check whether left is an address of a global variable >> +GlobalAddressDescription gdesc1, gdesc2; >> +if (GetGlobalAddressInformation(left, 0, )) { >> + if (GetGlobalAddressInformation(right - 1, 0, ) >> + && gdesc1.PointsInsideASameVariable (gdesc2)) >> +return; >> +} else { >> + // TODO > > Either goto do_error; here too, or do the if (offset <= 16384) case here. > Guess upstream wouldn't
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On Fri, Oct 13, 2017 at 02:53:50PM +0200, Martin Liška wrote: > @@ -3826,6 +3827,19 @@ pointer_diff (location_t loc, tree op0, tree op1) > pedwarn (loc, OPT_Wpointer_arith, >"pointer to a function used in subtraction"); > > + if (sanitize_flags_p (SANITIZE_POINTER_SUBTRACT)) > +{ > + gcc_assert (current_function_decl != NULL_TREE); > + > + op0 = save_expr (op0); > + op1 = save_expr (op1); > + > + tree tt = builtin_decl_explicit (BUILT_IN_ASAN_POINTER_SUBTRACT); > + *instrument_expr > + = build_call_expr_loc (loc, tt, 2, c_fully_fold (op0, false, NULL), > +c_fully_fold (op1, false, NULL)); > +} Why the c_fully_fold? Can't that be deferred until it actually is folded all together later? > + ret = pointer_diff (location, op0, op1, _expr); > goto return_build_binary_op; > } >/* Handle pointer minus int. Just like pointer plus int. */ > @@ -11707,6 +11721,18 @@ build_binary_op (location_t location, enum tree_code > code, > pedwarn (location, 0, > "comparison of distinct pointer types lacks a cast"); > } > + > + if (sanitize_flags_p (SANITIZE_POINTER_COMPARE)) > + { > + gcc_assert (current_function_decl != NULL_TREE); > + > + op0 = save_expr (op0); > + op1 = save_expr (op1); > + > + tree tt = builtin_decl_explicit (BUILT_IN_ASAN_POINTER_COMPARE); > + instrument_expr > + = build_call_expr_loc (location, tt, 2, op0, op1); > + } Is this the right spot for this? I mean then you don't handle ptr >= 0 or ptr > 0 and similar or ptr >= 0x12345678. I know we have warnings or pedwarns for those, still I think it would be better to handle the above e.g. before if ((TREE_CODE (TREE_TYPE (orig_op0)) == BOOLEAN_TYPE || truth_value_p (TREE_CODE (orig_op0))) ^ (TREE_CODE (TREE_TYPE (orig_op1)) == BOOLEAN_TYPE || truth_value_p (TREE_CODE (orig_op1 maybe_warn_bool_compare (location, code, orig_op0, orig_op1); by testing if ((code0 == POINTER_TYPE || code1 == POINTER_TYPE) && sanitize_flags_p (SANITIZE_POINTER_COMPARE)) What about the C++ FE? Or is pointer comparison well defined there? What about pointer subtraction? My memory is fuzzy. > +// { dg-options "-fsanitize=pointer-compare -O0" } Please use -fsanitize=address,pointer-compare etc. in the testcases, so that it is an example to users who don't know we have implicit -fsanitize=address for these tests. > + if (offset <= 2048) { > +if (__asan_region_is_poisoned (left, offset) == 0) I think the LLVM coding conventions don't want a space before ( above. > +// check whether left is a stack memory pointer > +if (GetStackVariableBeginning(left, _offset1)) { > + if (GetStackVariableBeginning(right - 1, _offset2) > + && shadow_offset1 == shadow_offset2) Nor && at the beginning of the line (they want it at the end of previous one). > + return; > + else > + goto do_error; > +} If you have goto do_error; for all cases, then you don't need to indent further stuff into else ... further and further. > +// check whether left is a heap memory address > +else { > + HeapAddressDescription hdesc1, hdesc2; > + if (GetHeapAddressInformation(left, 0, ) > + && hdesc1.chunk_access.access_type == kAccessTypeInside) { > + if (GetHeapAddressInformation(right, 0, ) > + && hdesc2.chunk_access.access_type == kAccessTypeInside > + && (hdesc1.chunk_access.chunk_begin > + == hdesc2.chunk_access.chunk_begin)) > + return; So, here one is a heap object and the other is not. That should be an do_error, right? > + } else { > + // check whether left is an address of a global variable > + GlobalAddressDescription gdesc1, gdesc2; > + if (GetGlobalAddressInformation(left, 0, )) { > + if (GetGlobalAddressInformation(right - 1, 0, ) > + && gdesc1.PointsInsideASameVariable (gdesc2)) > + return; > + } else { > + // TODO Either goto do_error; here too, or do the if (offset <= 16384) case here. Guess upstream wouldn't like it with a TODO spot. Jakub
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On 10/13/2017 01:17 PM, Jakub Jelinek wrote: > On Fri, Oct 13, 2017 at 01:01:37PM +0200, Martin Liška wrote: >> @@ -3826,6 +3827,18 @@ pointer_diff (location_t loc, tree op0, tree op1) >> pedwarn (loc, OPT_Wpointer_arith, >> "pointer to a function used in subtraction"); >> >> + if (sanitize_flags_p (SANITIZE_POINTER_SUBTRACT)) >> +{ >> + gcc_assert (current_function_decl != NULL_TREE); >> + >> + op0 = c_fully_fold (op0, false, NULL); >> + op1 = c_fully_fold (op1, false, NULL); >> + >> + tree tt = builtin_decl_explicit (BUILT_IN_ASAN_POINTER_SUBTRACT); >> + *instrument_expr >> += build_call_expr_loc (loc, tt, 2, op0, op1); >> +} > > If op0 or op1 have some embedded side-effects, won't you evaluate them > multiple times? I'd expect you need to c_save_expr them and make sure the > actual pointer difference expression uses the result of the save expr too. Good point, fixed. > >> --- a/libsanitizer/asan/asan_report.cc >> +++ b/libsanitizer/asan/asan_report.cc >> @@ -344,14 +344,68 @@ static INLINE void CheckForInvalidPointerPair(void >> *p1, void *p2) { >>if (!flags()->detect_invalid_pointer_pairs) return; >>uptr a1 = reinterpret_cast(p1); >>uptr a2 = reinterpret_cast(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; >> + >> + uptr offset = a1 < a2 ? a2 - a1 : a1 - a2; >> + uptr left = a1 < a2 ? a1 : a2; >> + uptr right = a1 < a2 ? a2 : a1; >> + if (offset <= 2048) { >> +if (__asan_region_is_poisoned (left, offset) == 0) >> + return; >> + } > > If offset <= 2048 and something is poisoned in between, then it is > clearly a failure, so you should either goto the error or duplicate > the two lines inside of the outer if above. Done that. > >> + >> + uptr shadow_offset1, shadow_offset2; >> + bool valid1, valid2; >> + { >> +ThreadRegistryLock l(()); >> +valid1 = GetStackVariableBeginning(left, _offset1); >>} >> + >> + // check whether a1 is a stack memory pointer >> + if (valid1) { >> +{ >> + ThreadRegistryLock l(()); >> + valid2 = GetStackVariableBeginning(right - 1, _offset2); > > Why do you take the registery lock twice? Just do: Yep, once is OK. However, we need to have the lock in a different scope than: GET_CALLER_PC_BP_SP; ReportInvalidPointerPair(pc, bp, sp, a1, a2); Otherwise we'll reach a deadlock. > { > ThreadRegistryLock l(()); > if (GetStackVariableBeginning(left, _offset1)) > { > if (GetStackVariableBeginning(right - 1, _offset2) && > shadow_offset1 == shadow_offset2) > return; > // goto do_error; or: > GET_CALLER_PC_BP_SP; > ReportInvalidPointerPair(pc, bp, sp, a1, a2); > return; > } > } > >> +} >> + >> +if (valid2 && shadow_offset1 == shadow_offset2) >> + return; >> + } >> + // check whether a1 is a heap memory address >> + else { >> +HeapAddressDescription hdesc1; >> +valid1 = GetHeapAddressInformation(a1, 0, ); >> + >> +if (valid1 && hdesc1.chunk_access.access_type == kAccessTypeInside) { >> + HeapAddressDescription hdesc2; >> + valid2 = GetHeapAddressInformation(a2, 0, ); > > Again, no need for valid1/valid2. Why do you use above left and right - 1 > and here a1 and a2? Shouldn't it be always left and right - 1? valid{1,2} removed. Now using a{1,2} just for error report purpose. Martin > > Jakub > >From 68710d309cedf737ef333e82f33a8f2c9fd43c25 Mon Sep 17 00:00:00 2001 From: marxinDate: Thu, 5 Oct 2017 12:14:25 +0200 Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}. gcc/ChangeLog: 2017-10-13 Martin Liska * doc/invoke.texi: Document the options. * flag-types.h (enum sanitize_code): Add SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. * ipa-inline.c (sanitize_attrs_match_for_inline_p): Add handling of SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. * opts.c: Define new sanitizer options. * sanitizer.def (BUILT_IN_ASAN_POINTER_COMPARE): Likewise. (BUILT_IN_ASAN_POINTER_SUBTRACT): Likewise. gcc/c/ChangeLog: 2017-10-13 Martin Liska * c-typeck.c (pointer_diff): Add new argument and instrument pointer subtraction. (build_binary_op): Similar for pointer comparison. gcc/testsuite/ChangeLog: 2017-10-13 Martin Liska * gcc.dg/asan/pointer-compare-1.c: New test. * gcc.dg/asan/pointer-compare-2.c: New test. * gcc.dg/asan/pointer-subtract-1.c: New test. * gcc.dg/asan/pointer-subtract-2.c: New test. --- gcc/c/c-typeck.c | 34 ++-- gcc/doc/invoke.texi
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On Fri, Oct 13, 2017 at 01:01:37PM +0200, Martin Liška wrote: > @@ -3826,6 +3827,18 @@ pointer_diff (location_t loc, tree op0, tree op1) > pedwarn (loc, OPT_Wpointer_arith, >"pointer to a function used in subtraction"); > > + if (sanitize_flags_p (SANITIZE_POINTER_SUBTRACT)) > +{ > + gcc_assert (current_function_decl != NULL_TREE); > + > + op0 = c_fully_fold (op0, false, NULL); > + op1 = c_fully_fold (op1, false, NULL); > + > + tree tt = builtin_decl_explicit (BUILT_IN_ASAN_POINTER_SUBTRACT); > + *instrument_expr > + = build_call_expr_loc (loc, tt, 2, op0, op1); > +} If op0 or op1 have some embedded side-effects, won't you evaluate them multiple times? I'd expect you need to c_save_expr them and make sure the actual pointer difference expression uses the result of the save expr too. > --- a/libsanitizer/asan/asan_report.cc > +++ b/libsanitizer/asan/asan_report.cc > @@ -344,14 +344,68 @@ static INLINE void CheckForInvalidPointerPair(void *p1, > void *p2) { >if (!flags()->detect_invalid_pointer_pairs) return; >uptr a1 = reinterpret_cast(p1); >uptr a2 = reinterpret_cast(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; > + > + uptr offset = a1 < a2 ? a2 - a1 : a1 - a2; > + uptr left = a1 < a2 ? a1 : a2; > + uptr right = a1 < a2 ? a2 : a1; > + if (offset <= 2048) { > +if (__asan_region_is_poisoned (left, offset) == 0) > + return; > + } If offset <= 2048 and something is poisoned in between, then it is clearly a failure, so you should either goto the error or duplicate the two lines inside of the outer if above. > + > + uptr shadow_offset1, shadow_offset2; > + bool valid1, valid2; > + { > +ThreadRegistryLock l(()); > +valid1 = GetStackVariableBeginning(left, _offset1); >} > + > + // check whether a1 is a stack memory pointer > + if (valid1) { > +{ > + ThreadRegistryLock l(()); > + valid2 = GetStackVariableBeginning(right - 1, _offset2); Why do you take the registery lock twice? Just do: { ThreadRegistryLock l(()); if (GetStackVariableBeginning(left, _offset1)) { if (GetStackVariableBeginning(right - 1, _offset2) && shadow_offset1 == shadow_offset2) return; // goto do_error; or: GET_CALLER_PC_BP_SP; ReportInvalidPointerPair(pc, bp, sp, a1, a2); return; } } > +} > + > +if (valid2 && shadow_offset1 == shadow_offset2) > + return; > + } > + // check whether a1 is a heap memory address > + else { > +HeapAddressDescription hdesc1; > +valid1 = GetHeapAddressInformation(a1, 0, ); > + > +if (valid1 && hdesc1.chunk_access.access_type == kAccessTypeInside) { > + HeapAddressDescription hdesc2; > + valid2 = GetHeapAddressInformation(a2, 0, ); Again, no need for valid1/valid2. Why do you use above left and right - 1 and here a1 and a2? Shouldn't it be always left and right - 1? Jakub
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On 10/12/2017 01:34 PM, Jakub Jelinek wrote: > On Thu, Oct 12, 2017 at 01:13:56PM +0200, Martin Liška wrote: >> + if (a1 == a2) >> +return; >> + >> + uptr shadow_offset1, shadow_offset2; >> + bool valid1, valid2; >> + { >> +ThreadRegistryLock l(()); >> + >> +valid1 = GetStackVariableBeginning(a1, _offset1); >> +valid2 = GetStackVariableBeginning(a2, _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; >> + uptr left = a1 < a2 ? a1 : a2; >> + if (offset <= 2048) { >> +if (__asan_region_is_poisoned (left, offset) == 0) >> + return; >> +else { >> + GET_CALLER_PC_BP_SP; >> + ReportInvalidPointerPair(pc, bp, sp, a1, a2); >> + return; >> +} > > My assumption was that this offset/left stuff would be done > right after the if (a1 == a2) above, i.e. after the most common > case - equality comparison, handle the case of pointers close to each other > without any expensive locking and data structure lookup (also, you only need > to compute left if offset is <= 2048). Only for larger distances, you'd > go through the other cases. I.e. see if one or both pointers point > into a stack variable, or heap, or global registered variable. > > For those 3 cases, I wonder if pairs of valid1 = ...; valid2 = ...; > is what is most efficient. What we really want is to error out if > one pointer is valid in any of the categories, but the other is > not. So, isn't the best general algorithm something like: > if (a1 == a2) > return; > if (distance <= 2048) > { > if (not poisoned area) > return; > } > else if (heap (a1)) > { > if (heap (a2) && same_heap_object) > return; > } > else if (stackvar (a1)) > { > if (stackvar (a2) && same_stackvar) > return; > } > else if (globalvar (a1)) > { > if (globalvar (a2) && same_globalvar) > return; > } > else > return; /* ??? We don't know what the pointers point to, punt. >Or perhaps try the not poisoned area check even for >slightly larger distances (like 16K) and only punt >for larger? */ > error; > > ? What order of the a1 tests should be done depends on how expensive > those tests are and perhaps some data gathering on real-world apps > on what pointers in the comparisons/subtracts are most common. > > Jakub > Thanks Jakub for valuable feedback. I'm sending new version where I implemented what you pointed. I also moved builtin created to FE and fixed quite some bugs I seen on postgres database. I guess I should slowly start with review process of libsanitizer changes. They are quite some. Martin >From 28401b05e8da21e57c8c0388fcc71fcbf34e0002 Mon Sep 17 00:00:00 2001 From: marxinDate: Thu, 5 Oct 2017 12:14:25 +0200 Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}. gcc/ChangeLog: 2017-10-13 Martin Liska * doc/invoke.texi: Document the options. * flag-types.h (enum sanitize_code): Add SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. * ipa-inline.c (sanitize_attrs_match_for_inline_p): Add handling of SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT. * opts.c: Define new sanitizer options. * sanitizer.def (BUILT_IN_ASAN_POINTER_COMPARE): Likewise. (BUILT_IN_ASAN_POINTER_SUBTRACT): Likewise. gcc/c/ChangeLog: 2017-10-13 Martin Liska * c-typeck.c (pointer_diff): Add new argument and instrument pointer subtraction. (build_binary_op): Similar for pointer comparison. gcc/testsuite/ChangeLog: 2017-10-13 Martin Liska * gcc.dg/asan/pointer-compare-1.c: New test. * gcc.dg/asan/pointer-compare-2.c: New test. * gcc.dg/asan/pointer-subtract-1.c: New test. * gcc.dg/asan/pointer-subtract-2.c: New test. --- gcc/c/c-typeck.c | 31 +-- gcc/doc/invoke.texi| 22 gcc/flag-types.h | 2 + gcc/ipa-inline.c | 8 ++- gcc/opts.c | 15 + gcc/sanitizer.def | 4 ++ gcc/testsuite/gcc.dg/asan/pointer-compare-1.c | 77 ++ gcc/testsuite/gcc.dg/asan/pointer-compare-2.c | 72 gcc/testsuite/gcc.dg/asan/pointer-subtract-1.c | 41 ++ gcc/testsuite/gcc.dg/asan/pointer-subtract-2.c | 32 +++
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On Thu, Oct 12, 2017 at 01:30:34PM +0200, Martin Liška wrote: > There's one false positive I've noticed: > > $ cat /tmp/ptr-cmp.c > int > __attribute__((noinline)) > foo(char *p1, char *p2) > { > if (p2 != 0 && p1 > p2) > return 0; > > return 1; > } Guess that is an argument for instrumenting pointer-compare/pointer-subtract earlier (in the FEs, perhaps into internal-fn). Because then it will have side-effects and thus folding (generic as well as during gimplification and on early gimple) will not do this kind of optimization with it. Of course you'd need to handle constexpr and folding in initializers then... Jakub
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On Thu, Oct 12, 2017 at 01:13:56PM +0200, Martin Liška wrote: > + if (a1 == a2) > +return; > + > + uptr shadow_offset1, shadow_offset2; > + bool valid1, valid2; > + { > +ThreadRegistryLock l(()); > + > +valid1 = GetStackVariableBeginning(a1, _offset1); > +valid2 = GetStackVariableBeginning(a2, _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; > + uptr left = a1 < a2 ? a1 : a2; > + if (offset <= 2048) { > + if (__asan_region_is_poisoned (left, offset) == 0) > + return; > + else { > + GET_CALLER_PC_BP_SP; > + ReportInvalidPointerPair(pc, bp, sp, a1, a2); > + return; > + } My assumption was that this offset/left stuff would be done right after the if (a1 == a2) above, i.e. after the most common case - equality comparison, handle the case of pointers close to each other without any expensive locking and data structure lookup (also, you only need to compute left if offset is <= 2048). Only for larger distances, you'd go through the other cases. I.e. see if one or both pointers point into a stack variable, or heap, or global registered variable. For those 3 cases, I wonder if pairs of valid1 = ...; valid2 = ...; is what is most efficient. What we really want is to error out if one pointer is valid in any of the categories, but the other is not. So, isn't the best general algorithm something like: if (a1 == a2) return; if (distance <= 2048) { if (not poisoned area) return; } else if (heap (a1)) { if (heap (a2) && same_heap_object) return; } else if (stackvar (a1)) { if (stackvar (a2) && same_stackvar) return; } else if (globalvar (a1)) { if (globalvar (a2) && same_globalvar) return; } else return; /* ??? We don't know what the pointers point to, punt. Or perhaps try the not poisoned area check even for slightly larger distances (like 16K) and only punt for larger? */ error; ? What order of the a1 tests should be done depends on how expensive those tests are and perhaps some data gathering on real-world apps on what pointers in the comparisons/subtracts are most common. Jakub
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
Hi. There's one false positive I've noticed: $ cat /tmp/ptr-cmp.c int __attribute__((noinline)) foo(char *p1, char *p2) { if (p2 != 0 && p1 > p2) return 0; return 1; } int main(int argc, char **argv) { return foo(argv[0], 0); } $ gcc /tmp/ptr-cmp.c -fsanitize=address,pointer-compare -O2 -fdump-tree-asan1=/dev/stdout && ./a.out __attribute__((noinline)) foo (char * p1, char * p2) { _Bool _1; _Bool _2; _Bool _3; _Bool _8; int _9; [100.00%] [count: INV]: _1 = p2_5(D) != 0B; __builtin___sanitizer_ptr_cmp (p2_5(D), p1_6(D)); _2 = p2_5(D) < p1_6(D); _3 = _1 & _2; _8 = ~_3; _9 = (int) _8; return _9; } ==31859==ERROR: AddressSanitizer: invalid-pointer-pair: 0x 0x7ffccadb4ff9 #0 0x400756 in foo (/home/marxin/Programming/postgres/src/pl/plpgsql/src/a.out+0x400756) #1 0x1513cde71f49 in __libc_start_main (/lib64/libc.so.6+0x20f49) #2 0x400689 in _start (/home/marxin/Programming/postgres/src/pl/plpgsql/src/a.out+0x400689) As I've been reading dump files, it's already in gimple dump: cat ptr-cmp.c.004t.gimple __attribute__((noinline)) foo (char * p1, char * p2) { int D.2181; _1 = p2 != 0B; _2 = p1 > p2; _3 = _1 & _2; if (_3 != 0) goto ; else goto ; ... Thoughts? Martin
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On 10/11/2017 04:22 PM, Jakub Jelinek wrote: > On Wed, Oct 11, 2017 at 03:36:40PM +0200, Martin Liška wrote: >>> 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. > > Ok. > >>> 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? > > First of all, why are you aligning anything? >> + uptr aligned_addr1 = addr1 & ~(SANITIZER_WORDSIZE/8 - 1); // align addr. >> + uptr aligned_addr2 = addr2 & ~(SANITIZER_WORDSIZE/8 - 1); // align addr. > You want to compare what the pointers point to, not what the aligned pointer > points to. > > Actually, looking around, there already is __sanitizer::mem_is_zero > which does what we want. > > Or even __asan_region_is_poisoned(addr1, addr2 - addr1). Hi. Thanks, the function works fine. I added various tests for global variables and found that my check with GetGlobalAddressInformation was wrong Thus I'm adding bool GlobalAddressDescription::PointsInsideASameVariable. Another change I did is: gcc -fsanitize=pointer-compare /tmp/main.c cc1: error: ‘-fsanitize=pointer-compare’ must be combined with ‘-fsanitize=address’ or ‘-fsanitize=kernel-address’ Which would make life easier in gcc.c, where one would have to distinguish between -fsanitize=pointer-compare and fsanitize=pointer-compare,kernel-address and according to -lasan will be added. Would it be possible to require it explicitly? I'm adding some numbers for postgres: 1 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/utils/mmgr/freepage.c:673 in FreePageBtreeCleanup 1 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/port/qsort_arg.c:168 in qsort_arg 2 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/regex/rege_dfa.c:316 in matchuntil 3 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/utils/mmgr/freepage.c:1543 in FreePageManagerPutInternal 4 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/access/gin/gindatapage.c:155 in GinDataLeafPageGetItems 4 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/common/base64.c:160 in pg_b64_decode 4 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/common/keywords.c:103 in ScanKeywordLookup 14 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/access/hash/hashovfl.c:768 in _hash_initbitmapbuffer 18 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/utils/mmgr/freepage.c:187 in FreePageManagerInitialize 26 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/port/qsort.c:188 in pg_qsort 40 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/utils/mmgr/dsa.c:1358 in init_span 43 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/utils/mmgr/freepage.c:1388 in FreePageManagerGetInternal 87 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/pl/plpgsql/src/pl_scanner.c:666 in plpgsql_location_to_lineno 92 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/port/qsort.c:168 in pg_qsort 154 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/storage/buffer/bufmgr.c:385 in ForgetPrivateRefCountEntry 273 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/storage/ipc/shm_toc.c:182 in shm_toc_insert 1158 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/regex/rege_dfa.c:153 in longest 3906 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/nodes/tidbitmap.c:840 in tbm_prepare_shared_iterate 6545 SUMMARY: AddressSanitizer: invalid-pointer-pair /home/marxin/Programming/postgres/src/backend/utils/adt/formatting.c:4582 in NUM_numpart_to_char There are some comparisons with NULL: ==28245==ERROR: AddressSanitizer: invalid-pointer-pair: 0x625000e42139 0x and quite some: Address 0x1465a178e5e0 is a wild pointer. Which is quite interesting reason, I'll investigate where a memory comes from. Thanks, Martin > > Jakub > >From
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On Wed, Oct 11, 2017 at 03:36:40PM +0200, Martin Liška wrote: > > 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. Ok. > > 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? First of all, why are you aligning anything? > + uptr aligned_addr1 = addr1 & ~(SANITIZER_WORDSIZE/8 - 1); // align addr. > + uptr aligned_addr2 = addr2 & ~(SANITIZER_WORDSIZE/8 - 1); // align addr. You want to compare what the pointers point to, not what the aligned pointer points to. Actually, looking around, there already is __sanitizer::mem_is_zero which does what we want. Or even __asan_region_is_poisoned(addr1, addr2 - addr1). Jakub
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
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 , 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(_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 &
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
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? 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. > --- 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 , 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. > + } > + > + BlockingMutexLock lock(_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 alignment) where say
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On 10/06/2017 03:33 PM, Jakub Jelinek wrote: > On Fri, Oct 06, 2017 at 02:46:05PM +0200, Martin Liška wrote: >> + if (sanitize_comparison_p) >> +{ >> + if (is_gimple_assign (s) >> + && gimple_assign_rhs_class (s) == GIMPLE_BINARY_RHS >> + && POINTER_TYPE_P (TREE_TYPE (gimple_assign_rhs1 (s))) >> + && POINTER_TYPE_P (TREE_TYPE (gimple_assign_rhs2 (s))) >> + && is_pointer_compare_opcode (gimple_assign_rhs_code (s))) > Hello. > Isn't it better to test is_pointer_compare_opcode right after > is_gimple_assign and leave the gimple_assign_rhs_class test out? Yes, it is :) > >> +{ >> + ptr1 = gimple_assign_rhs1 (s); >> + ptr2 = gimple_assign_rhs2 (s); >> + fn = BUILT_IN_ASAN_POINTER_COMPARE; >> +} >> + else if (gimple_code (s) == GIMPLE_COND >> + && POINTER_TYPE_P (TREE_TYPE (gimple_cond_lhs (s))) >> + && POINTER_TYPE_P (TREE_TYPE (gimple_cond_rhs (s))) >> + && is_pointer_compare_opcode (gimple_cond_code (s))) >> +{ >> + ptr1 = gimple_cond_lhs (s); >> + ptr2 = gimple_cond_rhs (s); >> + fn = BUILT_IN_ASAN_POINTER_COMPARE; >> +} > > You don't handle the case when there is a COND_EXPR with pointer comparison > in the condition. Good point, fixed. > >> +} >> + >> + if (sanitize_subtraction_p >> + && is_gimple_assign (s) >> + && gimple_assign_rhs_class (s) == GIMPLE_BINARY_RHS > > The above isn't really needed. > >> + && gimple_assign_rhs_code (s) == MINUS_EXPR) >> +{ >> + tree rhs1 = gimple_assign_rhs1 (s); >> + tree rhs2 = gimple_assign_rhs2 (s); >> + >> + if (TREE_CODE (rhs1) == SSA_NAME >> + || TREE_CODE (rhs2) == SSA_NAME) >> +{ >> + gassign *def1 >> += dyn_cast(SSA_NAME_DEF_STMT (rhs1)); >> + gassign *def2 >> += dyn_cast(SSA_NAME_DEF_STMT (rhs2)); >> + >> + if (def1 && def2 >> + && gimple_assign_rhs_class (def1) == GIMPLE_UNARY_RHS >> + && gimple_assign_rhs_class (def2) == GIMPLE_UNARY_RHS) >> +{ >> + if (POINTER_TYPE_P (TREE_TYPE (gimple_assign_rhs1 (def1))) >> + && POINTER_TYPE_P >> + (TREE_TYPE (gimple_assign_rhs1 (def2 > > Better add temporaries for rhs1/2 of def1, then you don't have issues with > too long lines. Yes. > >> --- a/gcc/doc/invoke.texi >> +++ b/gcc/doc/invoke.texi >> @@ -10935,6 +10935,26 @@ Enable AddressSanitizer for Linux kernel. >> See @uref{https://github.com/google/kasan/wiki} for more details. >> The option cannot be combined with @option{-fcheck-pointer-bounds}. >> >> +@item -fsanitize=pointer-compare >> +@opindex fsanitize=pointer-compare >> +Instrument comparison operation (<, <=, >, >=, -) with pointer operands. > > Poiinter-compare doesn't instrument -, so ", -" should be left out. That's typo. > >> +The option cannot be combined with @option{-fsanitize=thread} >> +and/or @option{-fcheck-pointer-bounds}. >> +Note: By default the check is disabled at run time. To enable it, >> +add @code{detect_invalid_pointer_pairs=1} to the environment variable >> +@env{ASAN_OPTIONS}. The checking currently works only for pointers >> allocated >> +on heap. >> + >> +@item -fsanitize=subtract > > -fsanitize=pointer-subtract > >> +@opindex fsanitize=pointer-compare > > s/compare/subtract/ Likewise. > >> +Instrument subtraction with pointer operands. >> +The option cannot be combined with @option{-fsanitize=thread} >> +and/or @option{-fcheck-pointer-bounds}. >> +Note: By default the check is disabled at run time. To enable it, >> +add @code{detect_invalid_pointer_pairs=1} to the environment variable >> +@env{ASAN_OPTIONS}. The checking currently works only for pointers >> allocated >> +on heap. >> + >> @item -fsanitize=thread >> @opindex fsanitize=thread >> Enable ThreadSanitizer, a fast data race detector. > > 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, >
Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.
On Fri, Oct 06, 2017 at 02:46:05PM +0200, Martin Liška wrote: > + if (sanitize_comparison_p) > + { > + if (is_gimple_assign (s) > + && gimple_assign_rhs_class (s) == GIMPLE_BINARY_RHS > + && POINTER_TYPE_P (TREE_TYPE (gimple_assign_rhs1 (s))) > + && POINTER_TYPE_P (TREE_TYPE (gimple_assign_rhs2 (s))) > + && is_pointer_compare_opcode (gimple_assign_rhs_code (s))) Isn't it better to test is_pointer_compare_opcode right after is_gimple_assign and leave the gimple_assign_rhs_class test out? > + { > + ptr1 = gimple_assign_rhs1 (s); > + ptr2 = gimple_assign_rhs2 (s); > + fn = BUILT_IN_ASAN_POINTER_COMPARE; > + } > + else if (gimple_code (s) == GIMPLE_COND > +&& POINTER_TYPE_P (TREE_TYPE (gimple_cond_lhs (s))) > +&& POINTER_TYPE_P (TREE_TYPE (gimple_cond_rhs (s))) > +&& is_pointer_compare_opcode (gimple_cond_code (s))) > + { > + ptr1 = gimple_cond_lhs (s); > + ptr2 = gimple_cond_rhs (s); > + fn = BUILT_IN_ASAN_POINTER_COMPARE; > + } You don't handle the case when there is a COND_EXPR with pointer comparison in the condition. > + } > + > + if (sanitize_subtraction_p > + && is_gimple_assign (s) > + && gimple_assign_rhs_class (s) == GIMPLE_BINARY_RHS The above isn't really needed. > + && gimple_assign_rhs_code (s) == MINUS_EXPR) > + { > + tree rhs1 = gimple_assign_rhs1 (s); > + tree rhs2 = gimple_assign_rhs2 (s); > + > + if (TREE_CODE (rhs1) == SSA_NAME > + || TREE_CODE (rhs2) == SSA_NAME) > + { > + gassign *def1 > + = dyn_cast(SSA_NAME_DEF_STMT (rhs1)); > + gassign *def2 > + = dyn_cast(SSA_NAME_DEF_STMT (rhs2)); > + > + if (def1 && def2 > + && gimple_assign_rhs_class (def1) == GIMPLE_UNARY_RHS > + && gimple_assign_rhs_class (def2) == GIMPLE_UNARY_RHS) > + { > + if (POINTER_TYPE_P (TREE_TYPE (gimple_assign_rhs1 (def1))) > + && POINTER_TYPE_P > + (TREE_TYPE (gimple_assign_rhs1 (def2 Better add temporaries for rhs1/2 of def1, then you don't have issues with too long lines. > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -10935,6 +10935,26 @@ Enable AddressSanitizer for Linux kernel. > See @uref{https://github.com/google/kasan/wiki} for more details. > The option cannot be combined with @option{-fcheck-pointer-bounds}. > > +@item -fsanitize=pointer-compare > +@opindex fsanitize=pointer-compare > +Instrument comparison operation (<, <=, >, >=, -) with pointer operands. Poiinter-compare doesn't instrument -, so ", -" should be left out. > +The option cannot be combined with @option{-fsanitize=thread} > +and/or @option{-fcheck-pointer-bounds}. > +Note: By default the check is disabled at run time. To enable it, > +add @code{detect_invalid_pointer_pairs=1} to the environment variable > +@env{ASAN_OPTIONS}. The checking currently works only for pointers allocated > +on heap. > + > +@item -fsanitize=subtract -fsanitize=pointer-subtract > +@opindex fsanitize=pointer-compare s/compare/subtract/ > +Instrument subtraction with pointer operands. > +The option cannot be combined with @option{-fsanitize=thread} > +and/or @option{-fcheck-pointer-bounds}. > +Note: By default the check is disabled at run time. To enable it, > +add @code{detect_invalid_pointer_pairs=1} to the environment variable > +@env{ASAN_OPTIONS}. The checking currently works only for pointers allocated > +on heap. > + > @item -fsanitize=thread > @opindex fsanitize=thread > Enable ThreadSanitizer, a fast data race detector. 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). > --- a/gcc/gcc.c > +++ b/gcc/gcc.c > @@ -971,7 +971,9 @@