-fsanitize=vptr instrumentation (take 2)

On Wed, Sep 17, 2014 at 10:27:02AM -0400, Jason Merrill wrote:
> On 09/16/2014 10:56 AM, Jakub Jelinek wrote:
> >vptr-5.C is one Jason mailed me yesterday, clang++ doesn't instrument this
> >and g++ right now doesn't either, build_static_cast_1 certainly isn't called
> >in that case, and I must say I have no idea what should be checked there,
> >where etc.
> 
> What needs to be checked is conversion (in this case implicit) to a virtual
> base; if the vptr doesn't point to a vtable that has the appropriate vbase
> offset, we should complain.
> 
> virtual base conversions are implemented in build_base_path under if
> (virtual_access).

Implemented now.

> >vptr-6.C shows where the this optimization is performed and where it isn't
> >(clang++ has 10 instrumentations in T::h and 1 in S::l, g++ has fewer than
> >that, but not 0 in T::h (1 in S::l is right and needed I think)).
> 
> I agree that 0 is enough for T::h and 1 for S::l.

Based on the further discussions, I've dropped the this optimizations,
perhaps we can tweak the sanopt pass to optimize some unnecessary checks
later on.

> >I hope all of f[1-6] is invalid, I really don't see how we could instrument
> >member accesses otherwise (we'd need to limit to not taking address of it);
> >NULL pointer shouldn't point at a valid object.

Based on the discussions, I'm not instrumenting &p->a or &*&p->a for now,
am instrumenting just p->a or *&p->a.  Until the WG decides what to do with
DR597/DR1531.

> I don't see anything in the standard saying that these are undefined, only
> that trying to access the (non-)object pointed to is undefined.  It would be
> undefined if a conversion to virtual base were involved, i.e.
> 
> struct V: virtual R { };
> 
> // undefined if p doesn't point to a V because of the conversion to
> // virtual base R
> int* f7 (V* p) { return &p->r; }

Yeah, this is instrumented now (including complaining if p is NULL here).

> These conditions were loosened in C++11 by DRs 597 and 1531; before that it
> was reasonable to regard f[1-6] as undefined, and perhaps clang is using the
> earlier interpretation.
> 
> >+  TREE_SIDE_EFFECTS (cond) = 1;
> ...
> >+  TREE_SIDE_EFFECTS (hash) = 1;
> 
> Why do you need to set TREE_SIDE_EFFECTS on these?

Not needed, dropped now.

> >+  if (current_function_decl == NULL_TREE
> >+      || lookup_attribute ("no_sanitize_undefined",
> >+			   DECL_ATTRIBUTES (current_function_decl)))
> >+    return NULL_TREE;
> 
> When would this be called outside a function?  If for namespace-scope
> variable initializers, I'd think we do want instrumentation.

Changed.

> >+  /* T t; t.foo (); doesn't need instrumentation, if the type is known.  */
> >+  if (is_addr
> >+      && TREE_CODE (op) == ADDR_EXPR
> >+      && DECL_P (TREE_OPERAND (op, 0))
> >+      && same_type_p (type,
> >+		      TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (op, 0)))))
> >+    return NULL_TREE;
> 
> You might want to use resolves_to_fixed_type_p in the optimizations.

For now I'd prefer not to do further optimizations, until it is clear that
those optimizations wouldn't remove any checks that could trigger on invalid
programs.

Here is an updated patch, ok if bootstrap/testing passes (so far just
checked with
make -j16 -k check RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} asan.exp tsan.exp ubsan.exp'
)?

I haven't implemented the upcast checks Alex Samsonov implemented in clang recently,
those aren't -fsanitize=vptr but -fsanitize=alignment and/or
-fsanitize=object-size, so can be done in an incremental patch.

Furthermore, not 100% sure there if say given:
struct S { int a; };
struct T : S { int b[1024]; };
S *foo (T *p) { return (S *) p; }
it is really undefined behavior if p (if not NULL) doesn't point to object
that has at least sizeof (T) bytes in it.  Where does C++ say that e.g.:
S *bar () { S s; return foo ((T *) &s); }
is undefined behavior (assuming the alignment is right)?
struct U { S s; int b[1024]; };
S *foo2 (U *p) { return (S *) p; }
S *bar2 () { S s; return foo2 ((U *) &s); }
is not instrumented and not undefined behavior (again, assuming the
alignment is right)?  I have in mind the common technique where even in GCC
sources e.g. for tree_node union we sometimes allocate smaller sizes (just
to cover the size of some union field), instead of whole union, or
allocating space just for parts of a structure (e.g. if there is a trailing
foo[1] array, allocation size can be done for offsetof(type, foo[0]) etc.).

2014-10-27  Jakub Jelinek  <jakub@redhat.com>

	* flag-types.h (enum sanitize_code): Add SANITIZE_VPTR,
	include SANITIZE_VPTR in SANITIZE_UNDEFINED.
	* opts.c (common_handle_option): Add -fsanitize=vptr.
	* sanitizer.def (BUILT_IN_UBSAN_HANDLE_DYNAMIC_TYPE_CACHE_MISS,
	BUILT_IN_UBSAN_HANDLE_DYNAMIC_TYPE_CACHE_MISS_ABORT): New.
	* ubsan.h (enum ubsan_null_ckind): Add UBSAN_DOWNCAST_POINTER,
	UBSAN_DOWNCAST_REFERENCE, UBSAN_UPCAST and UBSAN_CAST_TO_VBASE.
cp/
	* config-lang.in (gtfiles): Add cp/cp-ubsan.c.
	* cp-gimplify.c (cp_genericize_r): Call
	cp_ubsan_maybe_instrument_member_call for member calls and
	cp_ubsan_maybe_instrument_member_access for member accesses.
	(cp_ubsan_check_member_access_r): New function.
	(cp_genericize_tree): Call cp_walk_tree with
	cp_ubsan_check_member_access_r callback for -fsanitize=vptr.
	* cp-tree.h (cp_ubsan_maybe_instrument_member_call,
	cp_ubsan_maybe_instrument_member_access,
	cp_ubsan_maybe_instrument_downcast,
	cp_ubsan_maybe_instrument_cast_to_vbase,
	cp_ubsan_fixup_downcast_instrumentation): New prototypes.
	* cp-ubsan.c: New file.
	* Make-lang.in (CXX_AND_OBJCXX_OBJS): Add cp/cp-ubsan.o.
	* constexpr.c (cxx_eval_call_expression): Handle 4 argument
	BUILT_IN_UBSAN_HANDLE_DYNAMIC_TYPE_CACHE_MISS{,_ABORT}.
	* typeck.c (build_class_member_access_expr): Provide locus
	for COMPONENT_REFs.
	(build_static_cast_1): Instrument downcasts.
	* class.c (build_base_path): For -fsanitize=vptr and !fixed_type_p
	add ubsan instrumentation for virtual_access.
gcc/testsuite/
	* g++.dg/ubsan/vptr-1.C: New test.
	* g++.dg/ubsan/vptr-2.C: New test.
	* g++.dg/ubsan/vptr-3.C: New test.
	* g++.dg/ubsan/vptr-4.C: New test.
	* g++.dg/ubsan/vptr-5.C: New test.
	* g++.dg/ubsan/vptr-6.C: New test.
	* g++.dg/ubsan/vptr-7.C: New test.
	* g++.dg/ubsan/vptr-8.C: New test.
	* g++.dg/ubsan/vptr-9.C: New test.
libsanitizer/
	* ubsan/ubsan_handlers.cc (__ubsan::TypeCheckKinds): Cherry pick
	upstream r219642.

	Jakub

-fsanitize=vptr instrumentation (take 2)

Commit Message

Patch