[PATCH] Frontend: Avoid some UB by checking a FileChange's validity before a call

[Richard Smith]

I don't think this actually solves the problem:
InclusionRewriter::InclusionDirective also copies around a FileChange with
an uninitialized FileType, so I think you'll need to initialize it or give
it a copy constructor that doesn't copy the FileType until the Id has been
set.

Ultimately, it seems like the design here is at fault; maybe we should be
holding onto the HashLoc and Imported when we reach InclusionDirective, and
not inserting the value into the map until we reach FileChanged and know
the complete value we want to insert?

On Tue, Jun 30, 2015 at 4:58 PM, Justin Bogner <mail at justinbogner.com>
wrote:

> ping
>
> Justin Bogner <mail at justinbogner.com> writes:
> > If a FileChange has an invalid file, the FileType won't be initialized,
> > so it's undefined to pass it into Process like this. We immediately bail
> > out because the FileID is invalid in that case, so we can avoid the
> > issue by checking that first.
> >
> > The alternative is to initialize FileType in FileChange's constructor,
> > but it's not clear if there's a reasonable default. Should I do that
> > instead, or is this good to commit?
> >
> > commit bea2df0fed8f0ce08daa23d0dfc21aead1d12e63
> > Author: Justin Bogner <mail at justinbogner.com>
> > Date:   Mon Jun 22 12:45:00 2015 -0700
> >
> >     Frontend: Avoid some UB by checking a FileChange's validity before a
> call
> >
> >     If a FileChange has an invalid file, the FileType won't be
> >     initialized, so it's undefined to pass it into Process like this. We
> >     immediately bail out because the FileID is invalid in that case, so
> we
> >     can avoid the issue by checking that first.
> >
> > diff --git a/lib/Frontend/Rewrite/InclusionRewriter.cpp
> b/lib/Frontend/Rewrite/InclusionRewriter.cpp
> > index b9ea051..2ab18cf 100644
> > --- a/lib/Frontend/Rewrite/InclusionRewriter.cpp
> > +++ b/lib/Frontend/Rewrite/InclusionRewriter.cpp
> > @@ -439,7 +439,8 @@ bool InclusionRewriter::Process(FileID FileId,
> >                  WriteImplicitModuleImport(Change->Mod);
> >
> >                // else now include and recursively process the file
> > -              } else if (Process(Change->Id, Change->FileType)) {
> > +              } else if (!Change->Id.isInvalid() &&
> > +                         Process(Change->Id, Change->FileType)) {
> >                  // and set lineinfo back to this file, if the nested
> one was
> >                  // actually included
> >                  // `2' indicates returning to a file (after having
> included
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150630/246bc464/attachment.html>