Canonicalize the path provided by -fmodules-cache-path

[aprantl]

Summary

This fixes lookup mismatches that could previously happen when the module cache path contained a '/./' component.
In combination with https://reviews.llvm.org/D28299 this bug can cause a use-after-free.

rdar://problem/30413458

[bruno]

LGTM

lib/Frontend/CompilerInvocation.cpp

1434 ↗

(On Diff #91628)

I would suggest using FileManager::makeAbsolutePath, but at this point looks like we haven't set up a FileManager yet.

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

if (ClangResourceDir != "")
     llvm::sys::path::append(P, ClangResourceDir);
   else
     llvm::sys::path::append(P, "..", Twine("lib") + CLANG_LIBDIR_SUFFIX,
                             "clang", CLANG_VERSION_STRING);
 
   return P.str();
 }
 
-static void ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args) {
+static void ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
+                                  const std::string &WorkingDir) {
   using namespace options;
   Opts.Sysroot = Args.getLastArgValue(OPT_isysroot, "/");
   Opts.Verbose = Args.hasArg(OPT_v);
   Opts.UseBuiltinIncludes = !Args.hasArg(OPT_nobuiltininc);
   Opts.UseStandardSystemIncludes = !Args.hasArg(OPT_nostdsysteminc);
   Opts.UseStandardCXXIncludes = !Args.hasArg(OPT_nostdincxx);
   if (const Arg *A = Args.getLastArg(OPT_stdlib_EQ))
     Opts.UseLibcxx = (strcmp(A->getValue(), "libc++") == 0);
   Opts.ResourceDir = Args.getLastArgValue(OPT_resource_dir);
-  Opts.ModuleCachePath = Args.getLastArgValue(OPT_fmodules_cache_path);
+
+  // Canonicalize -fmodules-cache-path before storing it.
+  SmallString<128> P(Args.getLastArgValue(OPT_fmodules_cache_path));
+  if (!(P.empty() || llvm::sys::path::is_absolute(P))) {
+    if (WorkingDir.empty())
+      llvm::sys::fs::make_absolute(P);
+    else
+      llvm::sys::fs::make_absolute(WorkingDir, P);
+  }
+  llvm::sys::path::remove_dots(P);
+  Opts.ModuleCachePath = P.str();
+
   Opts.ModuleUserBuildPath = Args.getLastArgValue(OPT_fmodules_user_build_path);
   for (const Arg *A : Args.filtered(OPT_fprebuilt_module_path))
     Opts.AddPrebuiltModulePath(A->getValue());
   Opts.DisableModuleHash = Args.hasArg(OPT_fdisable_module_hash);
   Opts.ModulesHashContent = Args.hasArg(OPT_fmodules_hash_content);
   Opts.ModulesValidateDiagnosticOptions =
       !Args.hasArg(OPT_fmodules_disable_diagnostic_validation);
   Opts.ImplicitModuleMaps = Args.hasArg(OPT_fimplicit_module_maps);
bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
   ParseCommentArgs(LangOpts.CommentOpts, Args);
   ParseFileSystemArgs(Res.getFileSystemOpts(), Args);
   // FIXME: We shouldn't have to pass the DashX option around here
   InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags,
                                       LangOpts.IsHeaderFile);
   ParseTargetArgs(Res.getTargetOpts(), Args, Diags);
   Success &= ParseCodeGenArgs(Res.getCodeGenOpts(), Args, DashX, Diags,
                               Res.getTargetOpts());
-  ParseHeaderSearchArgs(Res.getHeaderSearchOpts(), Args);
+  ParseHeaderSearchArgs(Res.getHeaderSearchOpts(), Args,
+                        Res.getFileSystemOpts().WorkingDir);
   if (DashX == IK_AST || DashX == IK_LLVM_IR) {
     // ObjCAAutoRefCount and Sanitize LangOpts are used to setup the
     // PassManager in BackendUtil.cpp. They need to be initializd no matter
     // what the input type is.
     if (Args.hasArg(OPT_fobjc_arc))
       LangOpts.ObjCAutoRefCount = 1;
     // PIClevel and PIELevel are needed during code generation and this should be
     // set regardless of the input type.

cfe/trunk/test/Modules/Inputs/outofdate-rebuild/AppKit.h

+// AppKit
+#import "CoreVideo.h" // CoreVideo
+struct B { int i; };

cfe/trunk/test/Modules/Inputs/outofdate-rebuild/Cocoa.h

+// Cocoa
+#import "Foundation.h"
+#import "AppKit.h"
+
+struct A {  int i; };

cfe/trunk/test/Modules/Inputs/outofdate-rebuild/CoreText.h

+struct C { int i; };

cfe/trunk/test/Modules/Inputs/outofdate-rebuild/CoreVideo.h

+// CoreVideo
+#import "Foundation.h" // Foundation
+struct E { int i; };

cfe/trunk/test/Modules/Inputs/outofdate-rebuild/Foundation.h

+// Foundation
+#import "CoreText.h"
+struct D { int i; };

cfe/trunk/test/Modules/Inputs/outofdate-rebuild/module.modulemap

+module Cocoa {
+  header "Cocoa.h"
+}
+
+module AppKit {
+  header "AppKit.h"
+}
+
+module CoreText {
+  header "CoreText.h"
+}
+
+module Foundation {
+  header "Foundation.h"
+}
+
+module CoreVideo {
+  header "CoreVideo.h"
+}

cfe/trunk/test/Modules/modules-cache-path-canonicalization.m

+// RUN: rm -rf %t/cache %T/rel
+
+// This testcase reproduces a use-after-free after looking up a PCM in
+// a non-canonical modules-cache-path.
+//
+// Prime the module cache (note the '.' in the path).
+// RUN: %clang_cc1 -fdisable-module-hash -fmodules-cache-path=%t/./cache \
+// RUN:   -fmodules -fimplicit-module-maps -I %S/Inputs/outofdate-rebuild \
+// RUN:   %s -fsyntax-only
+//
+// Force a module to be rebuilt by creating a conflict.
+// RUN: echo "@import CoreText;" > %t.m
+// RUN: %clang_cc1 -DMISMATCH -Werror -fdisable-module-hash \
+// RUN:   -fmodules-cache-path=%t/./cache -fmodules -fimplicit-module-maps \
+// RUN:   -I %S/Inputs/outofdate-rebuild %t.m -fsyntax-only
+//
+// Rebuild.
+// RUN: %clang_cc1 -fdisable-module-hash -fmodules-cache-path=%t/./cache \
+// RUN:   -fmodules -fimplicit-module-maps -I %S/Inputs/outofdate-rebuild \
+// RUN:   %s -fsyntax-only
+
+
+// Unrelated to the above: Check that a relative path is resolved correctly.
+//
+// RUN: %clang_cc1 -working-directory %T/rel -fmodules-cache-path=./cache \
+// RUN:   -fmodules -fimplicit-module-maps -I %S/Inputs/outofdate-rebuild \
+// RUN:   -fdisable-module-hash %t.m -fsyntax-only -Rmodule-build 2>&1 \
+// RUN:   | FileCheck %s
+// CHECK: /rel/cache/CoreText.pcm
+@import Cocoa;