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

[Justin Bogner]

Richard Smith <richard at metafoo.co.uk> writes:
> 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?

You're right - the way the FileChange type was being used here was a bit
of a mess. I reworked all of this to simplify it and avoid needing
sometimes-initialized fields in r241140.

> 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