[PATCH] Sema: Fix some undefined behaviour when acting on redeclarations
Richard Smith
richard at metafoo.co.uk
Tue Jun 30 17:50:23 PDT 2015
On Tue, Jun 30, 2015 at 5:32 PM, Justin Bogner <mail at justinbogner.com>
wrote:
> Richard Smith <richard at metafoo.co.uk> writes:
> > On Tue, Jun 30, 2015 at 4:57 PM, Justin Bogner <mail at justinbogner.com>
> wrote:
> >
> > ping
> >
> > Justin Bogner <mail at justinbogner.com> writes:
> > > If we hit an error already, we may have set Name to nullptr, which
> > > means calling isAcceptableTagRedeclaration hits UB. Avoid this
> like we
> > > do elsewhere in the function by checking Name first.
> > >
> > > We could also fix this by passing OrigName instead, since the name
> is
> > > only used for diagnostics anyway. Should I do that instead, or is
> this
> > > good to commit?
> > >
> > > commit 203946970eafb48a120b29dfac1612b8762b9115
> > > Author: Justin Bogner <mail at justinbogner.com>
> > > Date: Mon Jun 22 00:05:05 2015 -0700
> > >
> > > Sema: Fix some undefined behaviour when acting on
> redeclarations
> > >
> > > If we hit an error already, we may have set Name to nullptr,
> which
> > > means calling isAcceptableTagRedeclaration hits UB. Avoid this
> like
> > we
> > > do elsewhere in the function by checking Name first.
> > >
> > > diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
> > > index ce89d99..8c94460 100644
> > > --- a/lib/Sema/SemaDecl.cpp
> > > +++ b/lib/Sema/SemaDecl.cpp
> > > @@ -11747,9 +11747,9 @@ Decl *Sema::ActOnTag(Scope *S, unsigned
> TagSpec,
> > TagUseKind TUK,
> > > SS.isNotEmpty() ||
> isExplicitSpecialization)) {
> > > // Make sure that this wasn't declared as an enum and now
> used
> > as a
> > > // struct or something similar.
> > > - if (!isAcceptableTagRedeclaration(PrevTagDecl, Kind,
> > > - TUK == TUK_Definition,
> KWLoc,
> > > - *Name)) {
> > > + if (Name && !isAcceptableTagRedeclaration(PrevTagDecl,
> Kind,
> > > + TUK ==
> > TUK_Definition, KWLoc,
> > > + *Name)) {
> >
> > So, this is tricky: if SafeToContinue is false, we don't want to carry on
> > because we'd make an EnumDecl a redeclaration of a RecordDecl or vicer
> versa,
> > which would violate the AST invariants. I'm not sure how we get here from
> > error recovery: both places that set Name to nullptr also jump past this
> code.
> > I think the only way to reach this is if SkipBody->Previous is non-null,
> in
> > which case this is not error recovery and we should perform this check.
> >
> > Can you instead change isAcceptableTagRedeclaration to take a const
> > IdentifierInfo*?
>
> The IdentifierInfo is only used in a diagnostic - I think it makes sense
> to pass `OrigName` here if we want the check. This is what we do a
> little below for diagnoseQualifiedDeclaration().
But OrigName is null too, isn't it? The only places that make Name and
OrigName differ also jump past this code.
> Also, do you have a testcase? Is this already caught by our testsuite +
> ubsan?
>
> Yep, this is caught by ubsan.
>
> > > bool SafeToContinue
> > > = (PrevTagDecl->getTagKind() != TTK_Enum &&
> > > Kind != TTK_Enum);
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150630/c8ec303a/attachment.html>
More information about the cfe-commits
mailing list