[PATCH] D30915: Canonicalize the path provided by -fmodules-cache-path

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL297790: Canonicalize the path provided by 
-fmodules-cache-path. (authored by adrian).

Changed prior to commit:
  https://reviews.llvm.org/D30915?vs=91628=91791#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D30915

Files:
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/Modules/Inputs/outofdate-rebuild/AppKit.h
  cfe/trunk/test/Modules/Inputs/outofdate-rebuild/Cocoa.h
  cfe/trunk/test/Modules/Inputs/outofdate-rebuild/CoreText.h
  cfe/trunk/test/Modules/Inputs/outofdate-rebuild/CoreVideo.h
  cfe/trunk/test/Modules/Inputs/outofdate-rebuild/Foundation.h
  cfe/trunk/test/Modules/Inputs/outofdate-rebuild/module.modulemap
  cfe/trunk/test/Modules/modules-cache-path-canonicalization.m

Index: cfe/trunk/test/Modules/Inputs/outofdate-rebuild/CoreText.h
===
--- cfe/trunk/test/Modules/Inputs/outofdate-rebuild/CoreText.h
+++ cfe/trunk/test/Modules/Inputs/outofdate-rebuild/CoreText.h
@@ -0,0 +1 @@
+struct C { int i; };
Index: cfe/trunk/test/Modules/Inputs/outofdate-rebuild/AppKit.h
===
--- cfe/trunk/test/Modules/Inputs/outofdate-rebuild/AppKit.h
+++ cfe/trunk/test/Modules/Inputs/outofdate-rebuild/AppKit.h
@@ -0,0 +1,3 @@
+// AppKit
+#import "CoreVideo.h" // CoreVideo
+struct B { int i; };
Index: cfe/trunk/test/Modules/Inputs/outofdate-rebuild/Foundation.h
===
--- cfe/trunk/test/Modules/Inputs/outofdate-rebuild/Foundation.h
+++ cfe/trunk/test/Modules/Inputs/outofdate-rebuild/Foundation.h
@@ -0,0 +1,3 @@
+// Foundation
+#import "CoreText.h"
+struct D { int i; };
Index: cfe/trunk/test/Modules/Inputs/outofdate-rebuild/Cocoa.h
===
--- cfe/trunk/test/Modules/Inputs/outofdate-rebuild/Cocoa.h
+++ cfe/trunk/test/Modules/Inputs/outofdate-rebuild/Cocoa.h
@@ -0,0 +1,5 @@
+// Cocoa
+#import "Foundation.h"
+#import "AppKit.h"
+
+struct A {  int i; };
Index: cfe/trunk/test/Modules/Inputs/outofdate-rebuild/module.modulemap
===
--- cfe/trunk/test/Modules/Inputs/outofdate-rebuild/module.modulemap
+++ cfe/trunk/test/Modules/Inputs/outofdate-rebuild/module.modulemap
@@ -0,0 +1,19 @@
+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"
+}
Index: cfe/trunk/test/Modules/Inputs/outofdate-rebuild/CoreVideo.h
===
--- cfe/trunk/test/Modules/Inputs/outofdate-rebuild/CoreVideo.h
+++ cfe/trunk/test/Modules/Inputs/outofdate-rebuild/CoreVideo.h
@@ -0,0 +1,3 @@
+// CoreVideo
+#import "Foundation.h" // Foundation
+struct E { int i; };
Index: cfe/trunk/test/Modules/modules-cache-path-canonicalization.m
===
--- cfe/trunk/test/Modules/modules-cache-path-canonicalization.m
+++ cfe/trunk/test/Modules/modules-cache-path-canonicalization.m
@@ -0,0 +1,30 @@
+// 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;
Index: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp
@@ -1419,7 +1419,8 @@
   return P.str();
 }
 
-static void ParseHeaderSearchArgs(HeaderSearchOptions , ArgList ) {
+static void ParseHeaderSearchArgs(HeaderSearchOptions , ArgList ,
+ 

[PATCH] D30915: Canonicalize the path provided by -fmodules-cache-path

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno accepted this revision.
bruno added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: lib/Frontend/CompilerInvocation.cpp:1434
+
+  // Canonicalize -fmodules-cache-path before storing it.
+  SmallString<128> P(Args.getLastArgValue(OPT_fmodules_cache_path));

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


https://reviews.llvm.org/D30915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30915: Canonicalize the path provided by -fmodules-cache-path

[Adrian Prantl via Phabricator via cfe-commits]

aprantl created this revision.

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


https://reviews.llvm.org/D30915

Files:
  lib/Frontend/CompilerInvocation.cpp
  test/Modules/Inputs/outofdate-rebuild/AppKit.h
  test/Modules/Inputs/outofdate-rebuild/Cocoa.h
  test/Modules/Inputs/outofdate-rebuild/CoreText.h
  test/Modules/Inputs/outofdate-rebuild/CoreVideo.h
  test/Modules/Inputs/outofdate-rebuild/Foundation.h
  test/Modules/Inputs/outofdate-rebuild/module.modulemap
  test/Modules/modules-cache-path-canonicalization.m

Index: test/Modules/modules-cache-path-canonicalization.m
===
--- /dev/null
+++ test/Modules/modules-cache-path-canonicalization.m
@@ -0,0 +1,30 @@
+// 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;
Index: test/Modules/Inputs/outofdate-rebuild/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/outofdate-rebuild/module.modulemap
@@ -0,0 +1,19 @@
+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"
+}
Index: test/Modules/Inputs/outofdate-rebuild/Foundation.h
===
--- /dev/null
+++ test/Modules/Inputs/outofdate-rebuild/Foundation.h
@@ -0,0 +1,3 @@
+// Foundation
+#import "CoreText.h"
+struct D { int i; };
Index: test/Modules/Inputs/outofdate-rebuild/CoreVideo.h
===
--- /dev/null
+++ test/Modules/Inputs/outofdate-rebuild/CoreVideo.h
@@ -0,0 +1,3 @@
+// CoreVideo
+#import "Foundation.h" // Foundation
+struct E { int i; };
Index: test/Modules/Inputs/outofdate-rebuild/CoreText.h
===
--- /dev/null
+++ test/Modules/Inputs/outofdate-rebuild/CoreText.h
@@ -0,0 +1 @@
+struct C { int i; };
Index: test/Modules/Inputs/outofdate-rebuild/Cocoa.h
===
--- /dev/null
+++ test/Modules/Inputs/outofdate-rebuild/Cocoa.h
@@ -0,0 +1,5 @@
+// Cocoa
+#import "Foundation.h"
+#import "AppKit.h"
+
+struct A {  int i; };
Index: test/Modules/Inputs/outofdate-rebuild/AppKit.h
===
--- /dev/null
+++ test/Modules/Inputs/outofdate-rebuild/AppKit.h
@@ -0,0 +1,3 @@
+// AppKit
+#import "CoreVideo.h" // CoreVideo
+struct B { int i; };
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -1419,7 +1419,8 @@
   return P.str();
 }
 
-static void ParseHeaderSearchArgs(HeaderSearchOptions , ArgList ) {
+static void ParseHeaderSearchArgs(HeaderSearchOptions , ArgList ,
+  const std::string ) {
   using namespace options;
   Opts.Sysroot = Args.getLastArgValue(OPT_isysroot, "/");
   Opts.Verbose = Args.hasArg(OPT_v);
@@ -1429,7 +1430,18 @@
   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))) {
+