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

[Richard Smith]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20121129/8288cfbc/attachment.html>