Page MenuHomePhabricator

Deprecate error prone temporary directory APIs
AbandonedPublic

Authored by davezarzycki on Jun 20 2020, 8:23 AM.

Details

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.

Diff Detail

Event Timeline

davezarzycki created this revision.Jun 20 2020, 8:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2020, 8:23 AM

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 added inline comments.Jun 21 2020, 4:31 PM
clang/lib/Driver/ToolChains/Clang.cpp
3177

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.

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.

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.

  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.

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 added inline comments.Jun 22 2020, 9:27 AM
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.

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

Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2020, 3:16 AM
davezarzycki abandoned this revision.Jun 26 2020, 7:52 AM

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.