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

[Aaron Ballman]

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