[PATCH] Sema: Fix some undefined behaviour when acting on redeclarations

[Justin Bogner]

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().

> 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 --------------
A non-text attachment was scrubbed...
Name: ub-clang-sema-2.patch
Type: text/x-patch
Size: 1056 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150630/771fcb28/attachment.bin>