Deprecate error prone temporary directory APIs

[davezarzycki]

Summary

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.

[lattner]

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.

[compnerd]

clang/lib/Driver/ToolChains/Clang.cpp

3177 ↗

(On Diff #272260)

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.

[davezarzycki]

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.

[sammccall]

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.

[davezarzycki]

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.

[compnerd]

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).

[aprantl]

lib/Driver/ToolChains/Clang.cpp

3176 ↗

(On Diff #272434)

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.

[davezarzycki]

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

[davezarzycki]

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.

lldb/source/Core/ModuleList.cpp

 //===-- ModuleList.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Interpreter/OptionValueFileSpec.h"
 #include "lldb/Interpreter/OptionValueFileSpecList.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Interpreter/Property.h"

 ModuleListProperties::ModuleListProperties() {
   m_collection_sp =
       std::make_shared<OptionValueProperties>(ConstString("symbols"));
   m_collection_sp->Initialize(g_modulelist_properties);
   m_collection_sp->SetValueChangedCallback(ePropertySymLinkPaths,
                                            [this] { UpdateSymlinkMappings(); });
 
   llvm::SmallString<128> path;
-  clang::driver::Driver::getDefaultModuleCachePath(path);
+  if (clang::driver::Driver::getDefaultModuleCachePath(path))
     SetClangModulesCachePath(path);
 }
 
 bool ModuleListProperties::GetEnableExternalLookup() const {
   const uint32_t idx = ePropertyEnableExternalLookup;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_modulelist_properties[idx].default_uint_value != 0);
 }
 

llvm/include/llvm/Support/FileSystem.h

 /// in your model to ensure this. Each '%' gives 4-bits of entropy so you can
 /// use 32 of them to get 128 bits of entropy.
 ///
 /// Example: clang-%%-%%-%%-%%-%%.s => clang-a0-b1-c2-d3-e4.s
 ///
 /// @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.
+LLVM_ATTRIBUTE_DEPRECATED(
     void createUniquePath(const Twine &Model, SmallVectorImpl<char> &ResultPath,
-                      bool MakeAbsolute);
+                          bool MakeAbsolute),
+    "Use createUniqueFile() or createUniqueDirectory()");
 
 /// Create a uniquely named file.
 ///
 /// Generates a unique path suitable for a temporary file and then opens it as a
 /// file. The name is based on \a Model with '%' replaced by a random char in
 /// [0-9a-f]. If \a Model is not an absolute path, the temporary file will be
 /// created in the current directory.
 ///

llvm/include/llvm/Support/Path.h

 /// "/var/tmp" or "C:/TEMP"
 ///
 /// @param erasedOnReboot Whether to favor a path that is erased on reboot
 /// rather than one that potentially persists longer. This parameter will be
 /// ignored if the user or system has set the typical environment variable
 /// (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<char> &result);
+LLVM_ATTRIBUTE_DEPRECATED(
+    void system_temp_directory(bool erasedOnReboot,
+                               SmallVectorImpl<char> &result),
+    "Only meant for debugging due to trivial security problems");
 
 /// Get the user's home directory.
 ///
 /// @param result Holds the resulting path name.
 /// @result True if a home directory is set, false otherwise.
 bool home_directory(SmallVectorImpl<char> &result);
 
 /// Get the directory where installed packages should put their

llvm/lib/Support/Path.cpp

 } // end unnamed namespace
 
 enum FSEntity {
   FS_Dir,
   FS_File,
   FS_Name
 };
 
+// Avoid a deprecation warning by moving the createUniquePath body here.
+static void _createUniquePath(const Twine &Model,
+                              SmallVectorImpl<char> &ResultPath,
+                              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 &Model, int &ResultFD,
                    SmallVectorImpl<char> &ResultPath, bool MakeAbsolute,
                    unsigned Mode, FSEntity Type,
                    sys::fs::OpenFlags Flags = sys::fs::OF_None) {
 
   // Limit the number of attempts we make, so that we don't infinite loop. E.g.
   // "permission denied" could be for a specific file (so we retry with a
   // different name) or for the whole directory (retry would always fail).
   // 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: {
       EC = sys::fs::openFileForReadWrite(Twine(ResultPath.begin()), ResultFD,
                                          sys::fs::CD_CreateNew, Flags, Mode);
       if (EC) {
         // errc::permission_denied happens on Windows when we try to open a file
         // that has been marked for deletion.
std::error_code getUniqueID(const Twine Path, UniqueID &Result) {
   if (EC)
     return EC;
   Result = Status.getUniqueID();
   return std::error_code();
 }
 
 void createUniquePath(const Twine &Model, SmallVectorImpl<char> &ResultPath,
                       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 &Model, int &ResultFd,
                                  SmallVectorImpl<char> &ResultPath,
                                  unsigned Mode) {
   return createUniqueEntity(Model, ResultFd, ResultPath, false, Mode, FS_File);
 }