On 09/17/2014 11:40 AM, Jakub Jelinek wrote:
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?
Well, it's a conversion to virtual base; I suppose we could use "member
access within" until the library is updated.
:(. 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?
That's my interpretation. It might be worth discussing with the clang
folks what their rationale is.
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?
Yes.
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?
I guess so. This passage is specifically about references to storage
either before or after the lifetime of an object, so it's not entirely
clear that it applies here, but I think it does.
And for the last one, should we before dynamic_cast verify the object
passed to dynamic_cast has the expected vptr?
Perhaps we should just add checking to the dynamic_cast code. I'm not
sure if that would slow it down enough to merit a separate entry point
for checking.
Jason
