[PATCH] D82259: Deprecate error prone temporary directory APIs

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki abandoned this revision.
davezarzycki added a comment.

Now that my core concern is addressed (moving clang's default module cache out 
of /tmp), I don't have the time to push for this deprecation. Sorry.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82259/new/

https://reviews.llvm.org/D82259



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

[PATCH] D82259: Deprecate error prone temporary directory APIs

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki updated this revision to Diff 272661.
davezarzycki added a comment.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Moved clang specific changes to: https://reviews.llvm.org/D82362


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82259/new/

https://reviews.llvm.org/D82259

Files:
  lldb/source/Core/ModuleList.cpp
  llvm/include/llvm/Support/FileSystem.h
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Path.cpp

Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -164,6 +164,41 @@
   FS_Name
 };
 
+// Avoid a deprecation warning by moving the createUniquePath body here.
+static void _createUniquePath(const Twine ,
+  SmallVectorImpl ,
+  bool MakeAbsolute) {
+  SmallString<128> ModelStorage;
+  Model.toVector(ModelStorage);
+
+  if (MakeAbsolute) {
+// Make model absolute by prepending a temp directory if it's not already.
+if (!sys::path::is_absolute(Twine(ModelStorage))) {
+  SmallString<128> TDir;
+#ifdef __clang__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+#endif
+  sys::path::system_temp_directory(true, TDir);
+#ifdef __clang__
+#pragma clang diagnostic pop
+#endif
+  sys::path::append(TDir, Twine(ModelStorage));
+  ModelStorage.swap(TDir);
+}
+  }
+
+  ResultPath = ModelStorage;
+  ResultPath.push_back(0);
+  ResultPath.pop_back();
+
+  // Replace '%' with random chars.
+  for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) {
+if (ModelStorage[i] == '%')
+  ResultPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
+  }
+}
+
 static std::error_code
 createUniqueEntity(const Twine , int ,
SmallVectorImpl , bool MakeAbsolute,
@@ -176,7 +211,7 @@
   // Checking which is racy, so we try a number of times, then give up.
   std::error_code EC;
   for (int Retries = 128; Retries > 0; --Retries) {
-sys::fs::createUniquePath(Model, ResultPath, MakeAbsolute);
+_createUniquePath(Model, ResultPath, MakeAbsolute);
 // Try to open + create the file.
 switch (Type) {
 case FS_File: {
@@ -778,28 +813,7 @@
 
 void createUniquePath(const Twine , SmallVectorImpl ,
   bool MakeAbsolute) {
-  SmallString<128> ModelStorage;
-  Model.toVector(ModelStorage);
-
-  if (MakeAbsolute) {
-// Make model absolute by prepending a temp directory if it's not already.
-if (!sys::path::is_absolute(Twine(ModelStorage))) {
-  SmallString<128> TDir;
-  sys::path::system_temp_directory(true, TDir);
-  sys::path::append(TDir, Twine(ModelStorage));
-  ModelStorage.swap(TDir);
-}
-  }
-
-  ResultPath = ModelStorage;
-  ResultPath.push_back(0);
-  ResultPath.pop_back();
-
-  // Replace '%' with random chars.
-  for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) {
-if (ModelStorage[i] == '%')
-  ResultPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
-  }
+  return _createUniquePath(Model, ResultPath, MakeAbsolute);
 }
 
 std::error_code createUniqueFile(const Twine , int ,
Index: llvm/include/llvm/Support/Path.h
===
--- llvm/include/llvm/Support/Path.h
+++ llvm/include/llvm/Support/Path.h
@@ -363,7 +363,10 @@
 /// (e.g., TEMP on Windows, TMPDIR on *nix) to specify a temporary directory.
 ///
 /// @param result Holds the resulting path name.
-void system_temp_directory(bool erasedOnReboot, SmallVectorImpl );
+LLVM_ATTRIBUTE_DEPRECATED(
+void system_temp_directory(bool erasedOnReboot,
+   SmallVectorImpl ),
+"Only meant for debugging due to trivial security problems");
 
 /// Get the user's home directory.
 ///
Index: llvm/include/llvm/Support/FileSystem.h
===
--- llvm/include/llvm/Support/FileSystem.h
+++ llvm/include/llvm/Support/FileSystem.h
@@ -800,8 +800,10 @@
 /// @param Model Name to base unique path off of.
 /// @param ResultPath Set to the file's path.
 /// @param MakeAbsolute Whether to use the system temp directory.
-void createUniquePath(const Twine , SmallVectorImpl ,
-  bool MakeAbsolute);
+LLVM_ATTRIBUTE_DEPRECATED(
+void createUniquePath(const Twine , SmallVectorImpl ,
+  bool MakeAbsolute),
+"Use createUniqueFile() or createUniqueDirectory()");
 
 /// Create a uniquely named file.
 ///
Index: lldb/source/Core/ModuleList.cpp
===
--- lldb/source/Core/ModuleList.cpp
+++ lldb/source/Core/ModuleList.cpp
@@ -82,8 +82,8 @@
[this] { UpdateSymlinkMappings(); });
 
   llvm::SmallString<128> path;
-  

[PATCH] D82259: Deprecate error prone temporary directory APIs

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:3176
+  if (llvm::sys::path::cache_directory(Result)) {
+llvm::sys::path::append(Result, "clang");
+llvm::sys::path::append(Result, "ModuleCache");

I would prefer to separate this out into its own patch. This is going to 
silently(!) break all sorts of scripts — for example many buildbots have a 
"clean module cache" action that searches for a directory with this name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82259/new/

https://reviews.llvm.org/D82259



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

[PATCH] D82259: Deprecate error prone temporary directory APIs

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

Thanks @davezarzycki!  Minor nit: git-clang-format the patch please, there was 
at least one linter warning in the changed code.

I do wonder if we can get away with the removal of the API though doing that in 
a follow up is pretty reasonable to me.

I think that one minor change is needed for this - an entry in the release 
notes would be nice to indicate to users that the cache paths have changed.  
Its difficult to say if anyone is depending on it being in `TMP`, but it is a 
user visible change to the driver (though they still have control via 
`-fmodules-cache-path`).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82259/new/

https://reviews.llvm.org/D82259



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

[PATCH] D82259: Deprecate error prone temporary directory APIs

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki updated this revision to Diff 272434.
davezarzycki added a comment.

1. Respect Darwin and XDG cache directories.
2. Drop atypical reverse DNS in the path.
3. Make clang more robust if the default module path cannot be determined.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82259/new/

https://reviews.llvm.org/D82259

Files:
  include/clang/Driver/Driver.h
  include/llvm/Support/FileSystem.h
  include/llvm/Support/Path.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Support/Path.cpp
  lib/Support/Unix/Path.inc
  source/Core/ModuleList.cpp
  test/Driver/modules-cache-path.m
  unittests/Driver/ModuleCacheTest.cpp

Index: lib/Support/Unix/Path.inc
===
--- lib/Support/Unix/Path.inc
+++ lib/Support/Unix/Path.inc
@@ -1133,19 +1133,6 @@
   return true;
 }
 
-bool cache_directory(SmallVectorImpl ) {
-  if (const char *RequestedDir = getenv("XDG_CACHE_HOME")) {
-result.clear();
-result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
-return true;
-  }
-  if (!home_directory(result)) {
-return false;
-  }
-  append(result, ".cache");
-  return true;
-}
-
 static bool getDarwinConfDir(bool TempDir, SmallVectorImpl ) {
   #if defined(_CS_DARWIN_USER_TEMP_DIR) && defined(_CS_DARWIN_USER_CACHE_DIR)
   // On Darwin, use DARWIN_USER_TEMP_DIR or DARWIN_USER_CACHE_DIR.
@@ -1171,6 +1158,27 @@
   return false;
 }
 
+bool cache_directory(SmallVectorImpl ) {
+#ifdef __APPLE__
+  if (getDarwinConfDir(false/*tempDir*/, result)) {
+return true;
+  }
+#else
+  // XDG_CACHE_HOME as defined in the XDG Base Directory Specification:
+  // http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
+  if (const char *RequestedDir = getenv("XDG_CACHE_HOME")) {
+result.clear();
+result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
+return true;
+  }
+#endif
+  if (!home_directory(result)) {
+return false;
+  }
+  append(result, ".cache");
+  return true;
+}
+
 static const char *getEnvTempDir() {
   // Check whether the temporary directory is specified by an environment
   // variable.
Index: lib/Support/Path.cpp
===
--- lib/Support/Path.cpp
+++ lib/Support/Path.cpp
@@ -164,6 +164,41 @@
   FS_Name
 };
 
+// Avoid a deprecation warning by moving the createUniquePath body here.
+static void _createUniquePath(const Twine , SmallVectorImpl ,
+  bool MakeAbsolute) {
+  SmallString<128> ModelStorage;
+  Model.toVector(ModelStorage);
+
+  if (MakeAbsolute) {
+// Make model absolute by prepending a temp directory if it's not already.
+if (!sys::path::is_absolute(Twine(ModelStorage))) {
+  SmallString<128> TDir;
+#ifdef __clang__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+#endif
+  sys::path::system_temp_directory(true, TDir);
+#ifdef __clang__
+#pragma clang diagnostic pop
+#endif
+  sys::path::append(TDir, Twine(ModelStorage));
+  ModelStorage.swap(TDir);
+}
+  }
+
+  ResultPath = ModelStorage;
+  ResultPath.push_back(0);
+  ResultPath.pop_back();
+
+  // Replace '%' with random chars.
+  for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) {
+if (ModelStorage[i] == '%')
+  ResultPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
+  }
+}
+
+
 static std::error_code
 createUniqueEntity(const Twine , int ,
SmallVectorImpl , bool MakeAbsolute,
@@ -176,7 +211,7 @@
   // Checking which is racy, so we try a number of times, then give up.
   std::error_code EC;
   for (int Retries = 128; Retries > 0; --Retries) {
-sys::fs::createUniquePath(Model, ResultPath, MakeAbsolute);
+_createUniquePath(Model, ResultPath, MakeAbsolute);
 // Try to open + create the file.
 switch (Type) {
 case FS_File: {
@@ -777,29 +812,8 @@
 }
 
 void createUniquePath(const Twine , SmallVectorImpl ,
-  bool MakeAbsolute) {
-  SmallString<128> ModelStorage;
-  Model.toVector(ModelStorage);
-
-  if (MakeAbsolute) {
-// Make model absolute by prepending a temp directory if it's not already.
-if (!sys::path::is_absolute(Twine(ModelStorage))) {
-  SmallString<128> TDir;
-  sys::path::system_temp_directory(true, TDir);
-  sys::path::append(TDir, Twine(ModelStorage));
-  ModelStorage.swap(TDir);
-}
-  }
-
-  ResultPath = ModelStorage;
-  ResultPath.push_back(0);
-  ResultPath.pop_back();
-
-  // Replace '%' with random chars.
-  for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) {
-if (ModelStorage[i] == '%')
-  ResultPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
-  }
+  bool MakeAbsolute) {
+  return _createUniquePath(Model, ResultPath, MakeAbsolute);
 }
 
 std::error_code createUniqueFile(const Twine , int ,
Index: include/llvm/Support/Path.h

[PATCH] D82259: Deprecate error prone temporary directory APIs

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki added a comment.

Sure, we can honor XDG_CACHE_DIR. Maybe as a followup, somebody can wire up 
Darwin's cache directory (which is retrievable via a BSD specific `confstr()` 
API with `_CS_DARWIN_USER_CACHE_DIR`). I'm not sure about other platforms.

I'll wait for more feedback before updating the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82259/new/

https://reviews.llvm.org/D82259



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

[PATCH] D82259: Deprecate error prone temporary directory APIs

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D82259#2106279 , @davezarzycki 
wrote:

> Sure, we can honor XDG_CACHE_DIR. Maybe as a followup, somebody can wire up 
> Darwin's cache directory (which is retrievable via a BSD specific `confstr()` 
> API with `_CS_DARWIN_USER_CACHE_DIR`). I'm not sure about other platforms.
>
> I'll wait for more feedback before updating the patch.


`llvm::sys::path::cache_directory` exists, it doesn't do anything special on 
Darwin (though would be the right place), but it does handle windows.

I'm not sure what the original motivation for `org.llvm.clang.$USER` format 
was, but it doesn't seem to be worth carrying over vs $CONFIG/clang/ to me - 
clang is well-known enough to "own" the name and most tools don't seem to add 
reverse-domains to these paths.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82259/new/

https://reviews.llvm.org/D82259



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

[PATCH] D82259: Deprecate error prone temporary directory APIs

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3177
   llvm::sys::path::append(Result, "ModuleCache");
 }
 

This default path is not ideal for Linux.  On Linux, if we want to put this 
into the home directory, we should follow XDG and put this into the 
`~/.cache/org.llvm.clang/ModuleCache`, respecting the environment variable 
`XDG_CACHE_DIR`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82259/new/

https://reviews.llvm.org/D82259



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

[PATCH] D82259: Deprecate error prone temporary directory APIs

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

Hi Dave,

This is doing two things - changing the behavior of clang (where it stores its 
module cache) and deprecating an API.  I'd recommend we start by focusing on 
whether the first part is the right thing to do, and just remove the API if 
this is the only client of it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82259/new/

https://reviews.llvm.org/D82259



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

[PATCH] D82259: Deprecate error prone temporary directory APIs

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki created this revision.
davezarzycki added reviewers: compnerd, aprantl, jakehehrlich, espindola, 
respindola, ilya-biryukov, pcc, sammccall.
davezarzycki added a project: LLVM.
Herald added subscribers: cfe-commits, hiraditya.
Herald added a project: clang.

Naive usage of shared temporary directories in Unix is a classic security 
problem. Let's deprecate two APIs that enable unsafe code and encourage people 
to use `createUniqueFile` or `createUniqueDirectory` instead.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82259

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  llvm/include/llvm/Support/FileSystem.h
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Path.cpp

Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -164,6 +164,41 @@
   FS_Name
 };
 
+// Avoid a deprecation warning by moving the createUniquePath body here.
+static void _createUniquePath(const Twine , SmallVectorImpl ,
+  bool MakeAbsolute) {
+  SmallString<128> ModelStorage;
+  Model.toVector(ModelStorage);
+
+  if (MakeAbsolute) {
+// Make model absolute by prepending a temp directory if it's not already.
+if (!sys::path::is_absolute(Twine(ModelStorage))) {
+  SmallString<128> TDir;
+#ifdef __clang__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+#endif
+  sys::path::system_temp_directory(true, TDir);
+#ifdef __clang__
+#pragma clang diagnostic pop
+#endif
+  sys::path::append(TDir, Twine(ModelStorage));
+  ModelStorage.swap(TDir);
+}
+  }
+
+  ResultPath = ModelStorage;
+  ResultPath.push_back(0);
+  ResultPath.pop_back();
+
+  // Replace '%' with random chars.
+  for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) {
+if (ModelStorage[i] == '%')
+  ResultPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
+  }
+}
+
+
 static std::error_code
 createUniqueEntity(const Twine , int ,
SmallVectorImpl , bool MakeAbsolute,
@@ -176,7 +211,7 @@
   // Checking which is racy, so we try a number of times, then give up.
   std::error_code EC;
   for (int Retries = 128; Retries > 0; --Retries) {
-sys::fs::createUniquePath(Model, ResultPath, MakeAbsolute);
+_createUniquePath(Model, ResultPath, MakeAbsolute);
 // Try to open + create the file.
 switch (Type) {
 case FS_File: {
@@ -777,29 +812,8 @@
 }
 
 void createUniquePath(const Twine , SmallVectorImpl ,
-  bool MakeAbsolute) {
-  SmallString<128> ModelStorage;
-  Model.toVector(ModelStorage);
-
-  if (MakeAbsolute) {
-// Make model absolute by prepending a temp directory if it's not already.
-if (!sys::path::is_absolute(Twine(ModelStorage))) {
-  SmallString<128> TDir;
-  sys::path::system_temp_directory(true, TDir);
-  sys::path::append(TDir, Twine(ModelStorage));
-  ModelStorage.swap(TDir);
-}
-  }
-
-  ResultPath = ModelStorage;
-  ResultPath.push_back(0);
-  ResultPath.pop_back();
-
-  // Replace '%' with random chars.
-  for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) {
-if (ModelStorage[i] == '%')
-  ResultPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
-  }
+  bool MakeAbsolute) {
+  return _createUniquePath(Model, ResultPath, MakeAbsolute);
 }
 
 std::error_code createUniqueFile(const Twine , int ,
Index: llvm/include/llvm/Support/Path.h
===
--- llvm/include/llvm/Support/Path.h
+++ llvm/include/llvm/Support/Path.h
@@ -363,7 +363,10 @@
 /// (e.g., TEMP on Windows, TMPDIR on *nix) to specify a temporary directory.
 ///
 /// @param result Holds the resulting path name.
-void system_temp_directory(bool erasedOnReboot, SmallVectorImpl );
+LLVM_ATTRIBUTE_DEPRECATED(
+void system_temp_directory(bool erasedOnReboot, SmallVectorImpl ),
+  "Only meant for debugging due to trivial security problems"
+);
 
 /// Get the user's home directory.
 ///
Index: llvm/include/llvm/Support/FileSystem.h
===
--- llvm/include/llvm/Support/FileSystem.h
+++ llvm/include/llvm/Support/FileSystem.h
@@ -800,8 +800,11 @@
 /// @param Model Name to base unique path off of.
 /// @param ResultPath Set to the file's path.
 /// @param MakeAbsolute Whether to use the system temp directory.
-void createUniquePath(const Twine , SmallVectorImpl ,
-  bool MakeAbsolute);
+LLVM_ATTRIBUTE_DEPRECATED(
+  void createUniquePath(const Twine , SmallVectorImpl ,
+bool MakeAbsolute),
+  "Use createUniqueFile() or createUniqueDirectory()"
+);
 
 /// Create a uniquely named file.
 ///
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++