Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

2017-12-21 Thread Martin Liška 
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}.

2017-12-18 Thread Martin Liška 
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}.

2017-12-05 Thread Jakub Jelinek 
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 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.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}.

2017-11-21 Thread Martin Liška 
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: marxin <mli...@suse.cz>
Date: Wed, 8 Nov 2017 13:16:17 +0100
Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

gcc/ChangeLog:

2017-11-21  Martin Liska  <mli...@suse.cz>

	* 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  <mli...@suse.cz>

	* 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  <mli...@suse.cz>

	* 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  <mli...@suse.cz>

	* 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<tree, va_gc> *, vec<tree, va_gc> *, 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_a

Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

2017-10-16 Thread Martin Liška 

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}.

2017-10-16 Thread Jakub Jelinek 
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}.

2017-10-16 Thread Martin Liška 
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: marxin <mli...@suse.cz>
Date: Thu, 5 Oct 2017 12:14:25 +0200
Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

gcc/ChangeLog:

2017-10-13  Martin Liska  <mli...@suse.cz>

	* 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  <mli...@suse.cz>

	* 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  <mli...@suse.cz>

	* 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  <mli...@suse.cz>

	* 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

Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

2017-10-16 Thread Jakub Jelinek 
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}.

2017-10-16 Thread Martin Liška 
ct 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 like it with a TODO spot.

Agree. Do you feel that it's right moment to trigger review process of 
libsanitizer
changes?

Thanks,
Martin

> 
>   Jakub
> 

>From 92cff481bc39feb9e17075d80e11774ea3085719 Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Thu, 5 Oct 2017 12:14:25 +0200
Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

gcc/ChangeLog:

2017-10-13  Martin Liska  <mli...@suse.cz>

	* 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  <mli...@suse.cz>

	* 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  <mli...@suse.cz>

	* 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  <mli...@suse.cz>

	* 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   | 57 +--
 libsanitizer/asan/asan_thread.cc   | 23 ++
 libsanitizer/asan/asan_thread.h|  3 +
 16 files changed, 458 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

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index cb9c589e061..d623418288f 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<tree, va_gc> *, vec<tree, va_gc> *, 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);
@@ -3779,10 +3779,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;
@@ -3826,6 +3827,17 @@ pointer_diff (location_t loc, tree op0, tree op1)
 pedwarn (loc, OPT_Wpointer_arith,
 	 "pointer to a function used in subtraction");
 
+  if (sanitize

Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

2017-10-13 Thread Jakub Jelinek 
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}.

2017-10-13 Thread Martin Liška 
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: marxin <mli...@suse.cz>
Date: Thu, 5 Oct 2017 12:14:25 +0200
Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

gcc/ChangeLog:

2017-10-13  Martin Liska  <mli...@suse.cz>

	* 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_SUBTR

Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

2017-10-13 Thread Jakub Jelinek 
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}.

2017-10-13 Thread Martin Liška 
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: marxin <mli...@suse.cz>
Date: Thu, 5 Oct 2017 12:14:25 +0200
Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

gcc/ChangeLog:

2017-10-13  Martin Liska  <mli...@suse.cz>

	* 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  <mli...@suse.cz>

	* 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  <mli...@suse.cz>

	* 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-inlin

Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

2017-10-12 Thread Jakub Jelinek 
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}.

2017-10-12 Thread Jakub Jelinek 
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}.

2017-10-12 Thread Martin Liška 
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}.

2017-10-12 Thread Martin Liška 
5e0 is a wild pointer.

Which is quite interesting reason, I'll investigate where a memory comes from.

Thanks,
Martin

> 
>   Jakub
> 

>From c83ea69668cc6c153024d648fb8ca565f8f16025 Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Thu, 5 Oct 2017 12:14:25 +0200
Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

gcc/ChangeLog:

2017-10-06  Martin Liska  <mli...@suse.cz>

	* asan.c (is_pointer_compare_opcode): New function.
	(instrument_pointer_comparison): Likewise.
	(asan_instrument): Handle SANITIZE_POINTER_COMPARE and
	SANITIZE_POINTER_SUBTRACT.
	* doc/invoke.texi: Document the options.
	* flag-types.h (enum sanitize_code): Add
	SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT.
	* opts.c: Define new sanitizer options.
	* sanitizer.def (BUILT_IN_ASAN_POINTER_COMPARE):
	(BUILT_IN_ASAN_POINTER_SUBTRACT): Likewise.
	* ipa-inline.c (sanitize_attrs_match_for_inline_p): Add handling
	of SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT.

gcc/testsuite/ChangeLog:

2017-10-06  Martin Liska  <mli...@suse.cz>

	* 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/asan.c | 119 +
 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  |  64 +
 gcc/testsuite/gcc.dg/asan/pointer-compare-2.c  |  47 ++
 gcc/testsuite/gcc.dg/asan/pointer-subtract-1.c |  41 +
 gcc/testsuite/gcc.dg/asan/pointer-subtract-2.c |  32 +++
 libsanitizer/asan/asan_descriptions.cc |  27 ++
 libsanitizer/asan/asan_descriptions.h  |   5 ++
 libsanitizer/asan/asan_report.cc   |  58 ++--
 libsanitizer/asan/asan_thread.cc   |  23 +
 libsanitizer/asan/asan_thread.h|   3 +
 15 files changed, 461 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-compare-1.c
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-compare-2.c
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-subtract-1.c
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-subtract-2.c

diff --git a/gcc/asan.c b/gcc/asan.c
index 2aa0a795af2..6bd437e0228 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -2370,6 +2370,122 @@ maybe_instrument_call (gimple_stmt_iterator *iter)
   return instrumented;
 }
 
+/* Return true if a given opcode CODE is potentially a non-valid comparison
+   of pointer types.  */
+
+static bool
+is_pointer_compare_opcode (tree_code code)
+{
+  return (code == LE_EXPR || code == LT_EXPR || code == GE_EXPR
+	  || code == GT_EXPR);
+}
+
+/* Instrument potential invalid operation executed on pointer types:
+   comparison different from != and == and subtraction of pointers.  */
+
+static void
+instrument_pointer_comparison (void)
+{
+  basic_block bb;
+  gimple_stmt_iterator i;
+
+  bool sanitize_comparison_p = sanitize_flags_p (SANITIZE_POINTER_COMPARE);
+  bool sanitize_subtraction_p = sanitize_flags_p (SANITIZE_POINTER_SUBTRACT);
+
+  FOR_EACH_BB_FN (bb, cfun)
+{
+  for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next ())
+	{
+	  gimple *s = gsi_stmt (i);
+
+	  tree ptr1 = NULL_TREE;
+	  tree ptr2 = NULL_TREE;
+	  enum built_in_function fn = BUILT_IN_NONE;
+
+	  if (sanitize_comparison_p)
+	{
+	  tree cond_expr, rhs1, rhs2, lhs, rhs;
+
+	  if (is_gimple_assign (s)
+		  && is_pointer_compare_opcode (gimple_assign_rhs_code (s))
+		  && (rhs1 = gimple_assign_rhs1 (s))
+		  && (rhs2 = gimple_assign_rhs2 (s))
+		  && POINTER_TYPE_P (TREE_TYPE (rhs1))
+		  && POINTER_TYPE_P (TREE_TYPE (rhs2)))
+		{
+		  ptr1 = rhs1;
+		  ptr2 = rhs2;
+		  fn = BUILT_IN_ASAN_POINTER_COMPARE;
+		}
+	  else if (is_gimple_assign (s)
+		   && gimple_assign_rhs_code (s) == COND_EXPR
+		   && (cond_expr = gimple_assign_rhs1 (s))
+		   && is_pointer_compare_opcode (TREE_CODE (cond_expr))
+		   && (rhs1 = TREE_OPERAND (cond_expr, 0))
+		   && (rhs2 = TREE_OPERAND (cond_expr, 1))
+		   && POINTER_TYPE_P (TREE_TYPE (rhs1))
+		   && POINTER_TYPE_P (TREE_TYPE (rhs2)))
+		{
+		  ptr1 = rhs1;
+		  ptr2 = rhs2;
+		  fn = BUILT_IN_ASAN_POINTER_COMPARE;
+		}
+	  else if (gimple_code (s) == GIMPLE_COND
+		   && (lhs = gimple_cond_lhs (s))
+		   && (rhs = gimple_cond_rhs (s))
+		   && POINTER_TYPE_P (TREE_TYPE (lhs))
+		   && POINTER_TYPE_P (TREE_TYPE (rhs))
+		   && is_pointer_compare_opcode (gimple_cond_code (s)))
+		{
+		  ptr1 = rhs;
+		  p

Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

2017-10-11 Thread Jakub Jelinek 
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}.

2017-10-11 Thread Martin Liška 
lesSame(uptr addr1, uptr addr2);
>>  
>>  const char *MaybeDemangleGlobalName(const char *name);
>>  void PrintGlobalNameIfASCII(InternalScopedString *str, const __asan_global 
>> );
>> 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
> 

>From 6bcf3d0064057edf55f0f65f7899b8a4a5e7e7dc Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Thu, 5 Oct 2017 12:14:25 +0200
Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

gcc/ChangeLog:

2017-10-06  Martin Liska  <mli...@suse.cz>

	* asan.c (is_pointer_compare_opcode): New function.
	(instrument_pointer_comparison): Likewise.
	(asan_instrument): Handle SANITIZE_POINTER_COMPARE and
	SANITIZE_POINTER_SUBTRACT.
	* doc/invoke.texi: Document the options.
	* flag-types.h (enum sanitize_code): Add
	SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT.
	* opts.c: Define new sanitizer options.
	* sanitizer.def (BUILT_IN_ASAN_POINTER_COMPARE):
	(BUILT_IN_ASAN_POINTER_SUBTRACT): Likewise.

gcc/testsuite/ChangeLog:

2017-10-06  Martin Liska  <mli...@suse.cz>

	* 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/asan.c | 119 +
 gcc/doc/invoke.texi|  18 
 gcc/flag-types.h   |   2 +
 gcc/opts.c |  10 +++
 gcc/sanitizer.def  |   4 +
 gcc/testsuite/gcc.dg/asan/pointer-compare-1.c  |  40 +
 gcc/testsuite/gcc.dg/asan/pointer-compare-2.c  |  41 +
 gcc/testsuite/gcc.dg/asan/pointer-subtract-1.c |  41 +
 gcc/testsuite/gcc.dg/asan/pointer-subtract-2.c |  32 +++
 libsanitizer/asan/asan_descriptions.cc |   9 ++
 libsanitizer/asan/asan_descriptions.h  |   1 +
 libsanitizer/asan/asan_globals.cc  |  29 ++
 libsanitizer/asan/asan_report.cc   |  52 +--
 libsanitizer/asan/asan_report.h|   1 +
 libsanitizer/asan/asan_thread.cc   |  23 +
 libsanitizer/asan/asan_thread.h|   1 +
 16 files changed, 416 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-compare-1.c
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-compare-2.c
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-subtract-1.c
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-subtract-2.c

diff --git a/gcc/asan.c b/gcc/asan.c
index 2aa0a795af2..6bd437e0228 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -2370,6 +2370,122 @@ maybe_instrument_call (gimple_stmt_iterator *iter)
   return instrumented;
 }
 
+/* Return true if a given opcode CODE is potentially a non-valid comparison
+   of pointer types.  */
+
+static bool
+is_pointer_compare_opcode (tree_code code)
+{
+  retu

Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

2017-10-11 Thread Jakub Jelinek 
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}.

2017-10-10 Thread Martin Liška 
t 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?

> 
>> --- a/gcc/gcc.c
>> +++ b/gcc/gcc.c
>> @@ -971,7 +971,9 @@ proper position among the other output files.  */
>>  /* Linker command line options for -fsanitize= early on the command line.  
>> */
>>  #ifndef SANITIZER_EARLY_SPEC
>>  #define SANITIZER_EARLY_SPEC "\
>> -%{!nostdlib:%{!nodefaultlibs:%{%:sanitize(address):" LIBASAN_EARLY_SPEC "} \
>> +%{!nostdlib:%{!nodefaultlibs:%{%:sanitize(address)\
>> +|%:sanitize(pointer-compare)\
>> +|%:sanitize(pointer-subtract):" LIBASAN_EARLY_SPEC "} \
>>  %{%:sanitize(thread):" LIBTSAN_EARLY_SPEC "} \
>>  %{%:sanitize(leak):" LIBLSAN_EARLY_SPEC "}}}"
>>  #endif
> 
> Depending on the above, I wonder if sanitize(address) shouldn't be implied
> by the new -fsanitize=pointer-{subtract,compare}, rather than polluting
> specs for it everywhere.

Agree.

> 
> 
>> -  if (flag_sanitize & SANITIZE_ADDRESS)
>> +  if (flag_sanitize & SANITIZE_ADDRESS
>> +  || flag_sanitize & SANITIZE_POINTER_COMPARE
>> +  || flag_sanitize & SANITIZE_POINTER_SUBTRACT)
> 
> Style nit, that would be better written as flag_sanitize & (A | B | C).
> But depending on the above it might be simpler.

Will not be needed as now we enable SANITIZE_ADDRESS.

> 
> In any case, I'm not sure if we want this patch in GCC until the library
> side isn't in so sorry state, because as we've discussed in the past, it
> isn't really usable.  At least my reading of it is that whenever you
> do one of these pointer comparisons or pointer subtractions and one or both
> of the pointers aren't heap allocated, you'll get a runtime error.

Fully agree, I will send libsanitizer bits to LLVM as soon as we're happy with
the shape of the patch.

Martin

> 
>   Jakub
> 

>From e43a7424cacde7f3cb7d3c1a481f9642d9cc8d95 Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Thu, 5 Oct 2017 12:14:25 +0200
Subject: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

gcc/ChangeLog:

2017-10-06  Martin Liska  <mli...@suse.cz>

	* asan.c (is_pointer_compare_opcode): New function.
	(instrument_pointer_comparison): Likewise.
	(asan_instrument): Handle SANITIZE_POINTER_COMPARE and
	SANITIZE_POINTER_SUBTRACT.
	* doc/invoke.texi: Document the options.
	* flag-types.h (enum sanitize_code): Add
	SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT.
	* opts.c: Define new sanitizer options.
	* sanitizer.def (BUILT_IN_ASAN_POINTER_COMPARE):
	(BUILT_IN_ASAN_POINTER_SUBTRACT): Likewise.

gcc/testsuite/ChangeLog:

2017-10-06  Martin Liska  <mli...@suse.cz>

	* 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/asan.c | 119 +
 gcc/doc/invoke.texi|  18 
 gcc/flag-types.h   |   2 +
 gcc/opts.c |   4 +
 gcc/sanitizer.def  |   4 +
 gcc/testsuite/gcc.dg/asan/pointer-compare-1.c  |  40 +
 gcc/testsuite/gcc.dg/asan/pointer-compare-2.c  |  41 +
 gcc/testsuite/gcc.dg/asan/pointer-subtract-1.c |  41 +
 gcc/testsuite/gcc.dg/asan/pointer-subtract-2.c |  32 +++
 libsanitizer/asan/asan_descriptions.cc |   9 ++
 libsanitizer/asan/asan_descriptions.h  |   1 +
 libsanitizer/asan/asan_globals.cc  |  25 ++
 libsanitizer/asan/asan_r

Re: [PATCH] Add -fsanitize=pointer-{compare,subtract}.

2017-10-06 Thread Jakub Jelinek 
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 @@

[PATCH] Add -fsanitize=pointer-{compare,subtract}.

2017-10-06 Thread Martin Liška 
Hi.

Adding a missing functionality mentioned and explained here:
https://github.com/google/sanitizers/wiki/AddressSanitizerClangVsGCC-(5.0-vs-7.1)#feature-8

Currently it only works for heap allocated variables. I'm working on support 
for stack and global
variables.

The functionality is not included in -fsanitize=address by default, one needs 
to explicitly ask
for both instrumentation and enabling in run-time. It's expensive check.

Ready to be installed?
Martin

gcc/ChangeLog:

2017-10-06  Martin Liska  

* asan.c (is_pointer_compare_opcode): New function.
(instrument_pointer_comparison): Likewise.
(asan_instrument): Handle SANITIZE_POINTER_COMPARE and
SANITIZE_POINTER_SUBTRACT.
(gate_asan): Likewise.
* doc/invoke.texi: Document the options.
* flag-types.h (enum sanitize_code): Add
SANITIZE_POINTER_COMPARE and SANITIZE_POINTER_SUBTRACT.
* gcc.c (SANITIZER_EARLY_SPEC): Handle
-fsanitize=pointer-compare and -fsanitize=pointer-subtract.
(SANITIZER_SPEC): Likewise.
(sanitize_spec_function): Likewise.
* opts.c (finish_options): Add checking for the options.
* sanitizer.def (BUILT_IN_ASAN_POINTER_COMPARE): New builtin.
(BUILT_IN_ASAN_POINTER_SUBTRACT): Likewise.
* toplev.c (process_options):  Handle SANITIZE_POINTER_COMPARE and
SANITIZE_POINTER_SUBTRACT.

gcc/testsuite/ChangeLog:

2017-10-06  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/asan.c | 104 -
 gcc/doc/invoke.texi|  20 +
 gcc/flag-types.h   |   2 +
 gcc/gcc.c  |  12 ++-
 gcc/opts.c |  10 +++
 gcc/sanitizer.def  |   4 +
 gcc/testsuite/gcc.dg/asan/pointer-compare-1.c  |  31 
 gcc/testsuite/gcc.dg/asan/pointer-compare-2.c  |  19 +
 gcc/testsuite/gcc.dg/asan/pointer-subtract-1.c |  31 
 gcc/testsuite/gcc.dg/asan/pointer-subtract-2.c |  19 +
 gcc/toplev.c   |   4 +-
 11 files changed, 253 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-compare-1.c
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-compare-2.c
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-subtract-1.c
 create mode 100644 gcc/testsuite/gcc.dg/asan/pointer-subtract-2.c


diff --git a/gcc/asan.c b/gcc/asan.c
index 2aa0a795af2..6feea017795 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -2370,6 +2370,104 @@ maybe_instrument_call (gimple_stmt_iterator *iter)
   return instrumented;
 }
 
+/* Return true if a given opcode CODE is potentially a non-valid comparison
+   of pointer types.  */
+
+static bool
+is_pointer_compare_opcode (tree_code code)
+{
+  return (code == LE_EXPR || code == LT_EXPR || code == GE_EXPR
+	  || code == GT_EXPR);
+}
+
+/* Instrument potential invalid operation executed on pointer types:
+   comparison different from != and == and subtraction of pointers.  */
+
+static void
+instrument_pointer_comparison (void)
+{
+  basic_block bb;
+  gimple_stmt_iterator i;
+
+  bool sanitize_comparison_p = sanitize_flags_p (SANITIZE_POINTER_COMPARE);
+  bool sanitize_subtraction_p = sanitize_flags_p (SANITIZE_POINTER_SUBTRACT);
+
+  FOR_EACH_BB_FN (bb, cfun)
+{
+  for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next ())
+	{
+	  gimple *s = gsi_stmt (i);
+
+	  tree ptr1 = NULL_TREE;
+	  tree ptr2 = NULL_TREE;
+	  enum built_in_function fn = BUILT_IN_NONE;
+
+	  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)))
+		{
+		  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;
+		}
+	}
+
+	  if (sanitize_subtraction_p
+	  && is_gimple_assign (s)
+	  && gimple_assign_rhs_class (s) == GIMPLE_BINARY_RHS
+	  && 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