Re: [RFC PATCH] -fsanitize=vptr instrumentation

[Jakub Jelinek <jakub at redhat dot com>]

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

build_base_path seems to be used in lots of places though, apparently
including member access, etc.  The ubsan library right now has just these
  const char *TypeCheckKinds[] = {                                                                                                                 
    "load of", "store to", "reference binding to", "member access within",                                                                         
    "member call on", "constructor call on", "downcast of", "downcast of"                                                                          
  };                                                                                                                                               
reasons for the runtime diagnostics (constructor call on, reference
binding to, load of and store to meant for other diagnostics), if what the
vptr-5.C testcase does for the pointer comparison? is not one of these, we'd
need to coordinate with upstream addition of other kinds (but,
build_base_path would need to be told what action it is, or it would need to
be instrumented in the callers of there like build_static_cast_1).
Suggestions on what the other kinds should be?

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

:(.  Well, for NULL one could argue that it was never a pointer to an object
and never will be, but as it could be non-NULL pointer refering to an out of
life object (e.g. deleted), I guess we have to stop instrumenting any
"member accesses" if it is surrounded by ADDR_EXPR, right?
So, while we'll instrument
  T *p;
...
  int i = p->a;
we won't be able to instrument
  int *ip = &p->a;
  int i = *ip;

Based on the DR597 resolution, I guess the only cases -fsanitize=vptr
can instrument are those listed in those bullets there:
- the pointer is used to access a non-static data member or call a non-static member function of the object, or
- the pointer is implicitly converted (4.10 [conv.ptr]) to a pointer to a virtual base class type, or
- the pointer is used as the operand of a static_cast (5.2.9 [expr.static.cast]) except when the conversion is to pointer to cv void, or to pointer to cv void and subsequently to pointer to cv char or pointer to cv unsigned char, or
- the pointer is used as the operand of a dynamic_cast (5.2.7 [expr.dynamic.cast])...

the first bullet is supposedly instrumented in the patch (except we
instrument ADDR_EXPR we shouldn't), the second, is that what vptr-5.C
above is about?, the third one is partially the build_static_cast_1,
but we only instrument downcasts, shouldn't we instrument upcasts too,
or static_cast conversions to say POD types other than to
void/char pointers?
And for the last one, should we before dynamic_cast verify the object
passed to dynamic_cast has the expected vptr?

> >+  TREE_SIDE_EFFECTS (cond) = 1;
> ...
> >+  TREE_SIDE_EFFECTS (hash) = 1;
> 
> Why do you need to set TREE_SIDE_EFFECTS on these?

I guess I can try to remove those, at some point I had in the patch
TRUTH_AND_EXPR instead of TRUTH_ANDIF_EXPR for the downcasts, and
some TREE_SIDE_EFFECTS I've added until I've noticed the missing IF.

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

Ok.  Then one won't be able to avoid the instrumentation there, unless
using -fno-sanitize=vptr, but there is really no place to stick that on.
> 
> >+  /* 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.

Ok, will have a look at that function.

	Jakub