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<gassign *>(SSA_NAME_DEF_STMT (rhs1));
> + gassign *def2
> + = dyn_cast<gassign *>(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 @@ 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.
> - 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.
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.
Jakub
