Re: [RFC PATCH] -fsanitize=vptr instrumentation

[Jason Merrill <jason@redhat.com>]

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

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

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

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

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?

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

> +  /* 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.

Jason