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

[Richard Smith]

On Fri, Jul 10, 2015 at 3:15 PM, Justin Bogner <mail at justinbogner.com>
wrote:

> Justin Bogner <mail at justinbogner.com> writes:
> > Justin Bogner <mail at justinbogner.com> writes:
> >> Richard Smith <richard at metafoo.co.uk> writes:
> >>> 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:
> >>>>>> +        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.
> >>
> >> You're right - I misread the assert where OrigName is assigned and was
> >> confused.
> >>
> >> I don't think passing const IdentifierInfo* is correct -
> >> isAcceptableTagRedeclaration implements [dcl.type.elab]p3, which is
> >> about elaborated-type-specifiers with the same name agreeing on kind.
> >> It doesn't make much sense to do this check if there isn't a name at
> >> all, and it seems suspicious to me that we're getting here at all.
> >>
> >> What does !Previous.empty() mean if we don't have a Name?
> >>
> >> This behaviour only triggers on one test in the test suite,
> >> test/Modules/submodules-merge-defs.cpp. You can reproduce with
> >> assert(Name) before the call to isAcceptableTagRedeclaration() if you're
> >> curious.
> >
> > FWIW, we're no longer hitting the UB here as of r241763 - I believe
> > r241743 is the relevant commit. It's not totally clear to me whether
> > that fixed or hid the bug though.
>
> Sorry, I was wrong about this, but now I understand how we get here.
> This fires when we're merging anonymous enum definitions (as per
> r236690). We're merging an enum like the following with a definition
> we've already seen:
>
>     enum { f, g, h };
>
> In this case, it's perfectly reasonable to not have a name and we do
> want to check that the redecl is compatible, so I've gone with the
> "const IdentifierInfo *" solution, attached. Okay to commit?
>

LGTM


> On a side note, passing null pointers to `operator<<(const
> DiagnosticBuilder, const IdentifierInfo)` eventually leads to this code:
>
>     case DiagnosticsEngine::ak_identifierinfo: {
>       const IdentifierInfo *II = getArgIdentifier(ArgNo);
>       assert(ModifierLen == 0 && "No modifiers for strings yet");
>
>       // Don't crash if get passed a null pointer by accident.
>       if (!II) {
>         const char *S = "(null)";
>         OutStr.append(S, S + strlen(S));
>         continue;
>       }
>
>       llvm::raw_svector_ostream(OutStr) << '\'' << II->getName() << '\'';
>       break;
>     }
>
> Here, the comment claims that passing a null pointer is an accident, but
> we seem to do it intentionally (at least, a few tests fail if I change
> it to an assert). Is the comment wrong, or should we look into fixing
> that?


Just looking through ActOnTag it seems there are a couple of different
diagnostics which can already produce such errors for anonymous
declarations. Ideally we should diagnose this better, though that won't be
completely trivial because it's not always reasonable to assume that a null
II means "<anonymous declaration>". So yes, we should fix this, but it's
unlikely we have good test coverage for these cases today so it may be a
little tricky to track them all down.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150710/0d2f7aef/attachment.html>