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

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

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

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

[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 
Date: 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}.

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

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

[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 
Date: 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}.

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

[Martin Liška]

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

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

[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 
Date: 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}.

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

[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 
Date: 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}.

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

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

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

[Martin Liška]

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

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

[Martin Liška]

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

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

[Martin Liška]

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

[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 @@