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