On Tue, Jun 30, 2015 at 5:32 PM, Justin Bogner <m...@justinbogner.com> wrote:
> Richard Smith <rich...@metafoo.co.uk> writes: > > On Tue, Jun 30, 2015 at 4:57 PM, Justin Bogner <m...@justinbogner.com> > wrote: > > > > ping > > > > Justin Bogner <m...@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 <m...@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@cs.uiuc.edu > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits