[modules][Patch][BugzillaID#20467] Diagnose if -fmodules-cache-path is not writeable

[Richard Smith]

+def err_module_cache_path_not_writable : Error<
+  "implicitly generated module cache %0 is not writable">,
+  DefaultFatal;

Don't we get here for both an implicit module cache and for an
explicitly-specified one?

On Thu, Sep 18, 2014 at 6:45 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> On Thu, Sep 18, 2014 at 7:50 AM, Vassil Vassilev
> <vasil.georgiev.vasilev at cern.ch> wrote:
> > On 09/17/2014 09:58 PM, Aaron Ballman wrote:
> >
> > On Wed, Sep 17, 2014 at 4:04 AM, Vassil Vassilev
> > <vasil.georgiev.vasilev at cern.ch> wrote:
> >
> > Hi,
> >   I am attaching a patch addressing llvm.org/bugs/show_bug.cgi?id=20467
> > Vassil
> >
> > Index: lib/Frontend/CompilerInstance.cpp
> > ===================================================================
> > --- lib/Frontend/CompilerInstance.cpp (revision 217432)
> > +++ lib/Frontend/CompilerInstance.cpp (working copy)
> > @@ -339,6 +339,15 @@
> >    if (!getHeaderSearchOpts().DisableModuleHash)
> >      llvm::sys::path::append(SpecificModuleCache,
> >                              getInvocation().getModuleHash());
> > +
> > +  // If the path is not writable we can't do anything but diagnose.
> > +  if (llvm::sys::fs::exists(SpecificModuleCache.str()) &&
> > +       !llvm::sys::fs::can_write(SpecificModuleCache.str())) {
> > +    SourceLocation NoLoc;
> > +    PP->getDiagnostics().Report(NoLoc,
> > diag::err_module_cache_path_not_writable)
> > +      << SpecificModuleCache;
> > +  }
> > +
> >
> > This has TOCTOU bugs -- the path could be writable at the point of
> > your check, but at the point which actually attempts to create a file
> > on that path can still fail. I think the correct way to handle this is
> > to handle the failure at the point of using the path, not attempting
> > to fail early.
> >
> > Thanks! IIUC a good place to do this would be
> > clang::HeaderSearch::getModuleCachePath ? Would that be acceptable?
>
> Not quite, because that would still suffer from the same TOCTOU bug. I
> was thinking more along the lines of in GlobalModuleIndex::writeIndex
> and GlobalModuleIndex::readIndex (or somewhere around there). When we
> go to open the file for reading/writing (create directories, etc),
> that action will fail. We should be diagnosing that once the failure
> occurs instead of preemptively.
>
> Basically, we shouldn't be doing:
>
> if (!can_write(blah))
>   report(why);
>
> fopen(blah);
>
> instead, we should be doing:
>
> fopen(blah);
> if (that_failed())
>   report(why);
>
> ~Aaron
> _______________________________________________
> 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/20140918/58fd478f/attachment.html>