[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D143418#4120058 <https://reviews.llvm.org/D143418#4120058>, @vedgy wrote:

> In D143418#4118905 <https://reviews.llvm.org/D143418#4118905>, @aaron.ballman wrote:
>
>> I don't think "tries hard" is a good enough guarantee for where files are placed. I'm not comfortable with the security posture of that approach as it could potentially leak sensitive information (try to override the temp directory to something that's chroot jailed, get sensitive files showing up in the system temp directory anyway).
>
> That's why the function's documentation explicitly doesn't guarantee anything. It should be safe to assume that security-sensitive users would at least read the documentation. If this function and potential future function like it are named specifically, a responsible security use case wouldn't be better off. The only safety advantage would be to those who don't bother reading the documentation. But why should we care much about such hypothetical careless security needs?

Security isn't a one-shot thing, but something you want in-depth defense for. Telling users "well you were careless about security so this is your fault" is still user-hostile even if you can point to documentation they should have read. (A whole lot of people use APIs without reading the documentation, unfortunately.)

>>> Does the multithreading issue mean that `clang_parseTranslationUnit_Impl()` can be called in a non-main thread and thus concurrently with `clang_CXIndex_setPreferredTempDirPath()`?
>>
>> Yes. However, I don't think that situation is officially supported (it's more that we don't want to knowingly make it harder to support in the future).
>
> All right. Let's do it via a new constructor then. Unfortunately, supporting different `CXIndexOptions` struct sizes/versions would complicate the libclang implementation and the libclang user code. But the alternative of adding a new constructor function each time can either grow to a large number of function parameters unneeded by most users or to an exponential number of overloads that support different usage requirements.
>
> How about including existing options, which //should// be set in constructor, in the initial struct version and deprecating the corresponding setters?

I think that makes a lot of sense.

>   typedef struct CXIndexOptions {
>     uint32_t size; // sizeof(struct CIndexOptions), used for option versioning
>     unsigned globalOptions;
>     const char *preferredTempDirPath;

In terms of documenting the structure, should we document this member as only impacting preamble files currently, should we rename this to be preamble-path specific, or should we try to use this preferred path in all the places we need the temp directory?

(I lean towards renaming to preamble-path specific -- then users don't have the problem of upgrading to a newer CIndex potentially changing the behavior of where non-preamble files are stored, and we don't have to do the work up-front to audit other temp file uses.)

>   const char *invocationEmissionPath;
>
> } CXIndexOptions;
>
>   The naming of struct data members is inconsistent in libclang's Index.h. They start with a lower-case letter in about half of the structs. Which naming scheme should the members of the new struct use?

Wow, it's almost an even split between styles from what I'm seeing, that's lovely. Let's go with the LLVM coding style recommendation because there's not a clear "winner" used in the same file. (So start with an uppercase letter instead of lower case.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418