<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 30, 2015 at 4:57 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">ping<br>
<div><div class="h5"><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>
</div></div>> 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>
<span class="">><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>
</span>> 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, TagUseKind TUK,<br>
>                          SS.isNotEmpty() || isExplicitSpecialization)) {<br>
>          // Make sure that this wasn't declared as an enum and now used 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 == TUK_Definition, KWLoc,<br>
> +                                                  *Name)) {<br></blockquote><div><br></div><div>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.</div><div><br></div><div>Can you instead change isAcceptableTagRedeclaration to take a const IdentifierInfo*?</div><div><br></div><div><br></div><div>Also, do you have a testcase? Is this already caught by our testsuite + ubsan?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
>            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>
</blockquote></div><br></div></div>