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
