[LLVMdev] [cfe-dev] UB in TypeLoc casting

[Richard Smith]

On Wed, Dec 5, 2012 at 1:06 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Thu, Nov 29, 2012 at 8:16 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > On Thu, Nov 29, 2012 at 7:43 PM, Eli Friedman <eli.friedman at gmail.com>
> > wrote:
> >>
> >> On Thu, Nov 29, 2012 at 3:49 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >> > Moving to LLVM dev to discuss the possibility of extending the cast
> >> > infrastructure to handle this.
> >> >
> >> > On Tue, Nov 20, 2012 at 5:51 PM, John McCall <rjmccall at apple.com>
> wrote:
> >> >> On Nov 18, 2012, at 5:05 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >> >>> TypeLoc casting looks bogus.
> >> >>>
> >> >>> TypeLoc derived types return true from classof when the dynamic type
> >> >>> is not actually an instance of the derived type. The TypeLoc is then
> >> >>> (erroneously) cast to the derived type and the derived type's
> implicit
> >> >>> copy ctor is executed (so long as you remember to assign the result
> of
> >> >>> cast<SpecificTypeLoc>), if you copy, otherwise the SpecificTypeLoc's
> >> >>> member functions are actually invoked on a TypeLoc object - bogas,
> but
> >> >>> it works (because there's no other members in the SpecificTypeLoc
> >> >>> types).
> >> >>
> >> >> Yep.  See also LLVM's IntrinsicInst.  This kind of UB is very common
> >> >> and
> >> >> essentially harmless, but if you want to stamp it out, feel free.
> >> >>
> >> >>> Richard / Kostya: what's the word on catching this kind of UB
> >> >>> (essentially: calling member functions of one type on an instance
> not
> >> >>> of that type)? (in the specific case mentioned below, as discussed
> in
> >> >>> the original thread, ASan or MSan might be able to help)
> >> >>>
> >> >>> Clang Dev: what should we do to fix this? I don't think it's helpful
> >> >>> to add machinery to llvm::cast to handle concrete type conversion,
> so
> >> >>> I think we should consider a non-llvm::cast solution for this
> >> >>> hierarchy. Replace the llvm::isa calls with getTypeLocClass calls
> (or
> >> >>> a separate wrapper for that) & add a SpecificTypeLoc(const TypeLoc&)
> >> >>> ctor for every specific TypeLoc that has an appropriate assert &
> calls
> >> >>> up through to the base copy ctor. It'll be a fair bit of code to add
> >> >>> to TypeLoc, but nothing complicated.
> >> >>>
> >> >>> Any other ideas?
> >> >>
> >> >> I don't see why isa<> and cast<> aren't the right code interface for
> >> >> this,
> >> >> rather than reinventing something else.  You just need to teach
> cast<>
> >> >> to construct and return a proper temporary.
> >> >
> >> > LLVM-dev folks: what do you reckon? Should we extend the cast
> >> > infrastructure to be able to handle value type conversions?
> >>
> >> No, I don't think that makes sense; the casting infrastructure is
> >> complicated enough without having to deal with this.  It would be easy
> >> enough to implement a method on TypeLoc to handle this,
> >>
> >> > It doesn't really feel like the right fit to me, but I'll defer to the
> >> > community's wisdom if it's overwhelming. I'd just like to see a
> >> > solution wherein we can't write this particular kind of bug again,
> >> > ideally.
> >>
> >> Attaching proof-of-concept solution which prevents this kind of bug.
> >> (Ugly, and requires C++11 support as written, but potentially
> >> interesting anyway.)
> >
> >
> > The C++11 should be easy to remove (move the enable_if to the return
> type).
> > This approach seems reasonable to me.
>
> Thanks Eli - that does seem to do the trick (took me a little while to
> wrap my head around it, though). I've modified it, as Richard
> suggested, to use enable_if in the return type & it builds in C++98
> and correctly catches the TypeLoc usage.
>
> I'll have to find some time to actually go & fix TypeLoc before we can
> submit this, though.
>
> (& technically we could allow casting where the parameter type is a
> temporary, so long as you only cast to the same type - but that's not
> much use. I wonder if we should trivially disallow casting when the
> source/dest type are the same anyway? But I suppose there's no harm in
> allowing that either, except that it might confuse the reader a bit
> when they see a no-op cast like that)


ISTR we have cases of no-op casts in templates and in code generated by x
macros. These seem reasonably harmless.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20121205/0987ea60/attachment.html>