[PATCH] D92155: Load plugins when creating a CompilerInvocation.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

TL;DR: clangd has some different concerns than clang, so we really need to approach this as a new feature with some design required (even if a mechanism already exists).
The most promising ideas I can think of involve doing the loading at clangd startup based on some trusted configuration, and *not* dynamically loading in the driver, instead allowing the use of plugins already loaded.

In D92155#2418884 <https://reviews.llvm.org/D92155#2418884>, @psionic12 wrote:

>> Right, I understand (a little bit, at least!) what plugins *can* do. What I'm asking specifically is: this feature has a cost, how important is supporting it? Are there codebases where these attributes are widely used, and enforcement at development time is particularly valuable?
>
> Well, I can't tell you how widely plugin used, because they often used in third-party projects. Firefox team seems use them a lot, the `ParsedAttrInfo` plugin implementation seems write by them if I remember right. And our team use both plugins and clangd a lot, that's why I'm willing to contribute to make them work together. As about the importance, my personal opinion is that since there's a feature, we should try to polish it...

Ah, thanks! If the widely-used plugins live in-tree, then it's more plausible to support just those, e.g.

- we don't need to consider the security concerns of loading unknown code
- we can fix them if they become e.g. thread-hostile

>> Sorry, I probably should have said "can plugins be thread-hostile".
>> Clangd runs several parsing actions in parallel, each on a single separate thread.
>> If a plugin assumes there's only a single instance of it running and uses static data accordingly, we're going to hit problems in clangd. 
>> The problem is that precisely because clang uses a single thread, (some) plugins may think this is a reasonable assumption. And it's not possible to tell "from the outside" whether a given plugin is unsafe in this way.
>
> That seems a serious problem, but I got a question, what if the clang itself declared some static data (I'll investigate later, just ask for now)?

You mean if clang itself were thread-hostile? We've found and fixed such problems over the years as part of developing clangd and other tools that use the clang frontend in a multithreaded environment. This cost some time and effort, but had a high reward, we had access to all the code, and the problem was bounded. So it was feasible and worthwhile!

>> It looks like this patch moves `LoadLibraryPermanently` to a different location, per the patch description clangd does not currently load plugins. That's why I'm concerned this patch may introduce unsafe loading of untrusted .sos.
>
> Well, since it called plugin, it's users choose to use it or not, I personally think we can't check if an `.so` is trusted, and neither should we care...

We need to be careful about what "user" and "choose" means.
e.g. if you clone an untrusted git repo and open `src/foo.cpp` in your editor...
(and unknown to you, `src/compile_flags.txt` contains `-plugin=../lib/pwn.so`)
(and unknown to you, `lib/pwn.so` wipes your hard disk when loaded)
Have you made a meaningful choice to use that plugin? That's the security context clangd is in - the decision to parse the file is implicit, and the compile flags are potentially attacker-controlled.

While the *clang* flags to parse a particular file are attacker-controlled, the *clangd* flags are not, and are a viable place to put security-sensitive configuration. So one potential design idea is to pass `-clang-plugin=foo.so` to clangd, and load foo.so on startup (not in the clang driver). Then when it comes time to parse a file, we can allow its args to activate plugins that are already loaded, but not to load new ones.
(There's precedent for this: the clangd --query-driver flag is designed to mitigate a similar clang-flags-lead-to-arbitary-code-execution concern)

I think this might be viable but as you can imagine it's considerably more involved than just enabling the plugin code in the driver.

>> - the filesystem access to the libraries isn't virtualized (vfs) and I don't know if it really can be.
>
> Sorry I still can't understand this, could you help to explain more?

clangd (and other clang tools) get deployed in environments where all access to the filesystem goes through a llvm::vfs::Filesystem, and all filenames referred to in the compile command are within that virtual filesystem rather than the real one.
(Again, this isn't an issue if we require these paths to be specified on the *clangd* startup command line, as opposed to the clang flags that form the compile command)

> What I mean before is that since this part of code is used in clang driver as well, it should be no problem get called by clangd.

To my knowledge, clang itself is never used in such an environment, so only the parts that already run in clang tools have been made VFS-clean.

>>>> - consequences of bad behavior/crash are worse, because clangd is long-running
>>>
>>> Does clangd shares the same process with an `FrontendAction`? Because in clang, if a plugin carshes, the whole process does terminated, clang didn't handle this yet(just a core dump).
>>
>> Yes. And I'm not convinced it's possible to handle this safely without large architectural changes.
>
> Well, I guess currently I can do nothing to improve this, but if a user uses a plugin, it crashed, that's should be the plugin's problem...

I'm not sure that expecting everyone writing clang plugins to understand and care about clangd's quirks is reasonable or realistic. If we set the expectation that plugins should work, there will be some pressure to "emulate" clang sufficiently accurately. We do this for clang-tidy checks (but again, the value there is very high).

>> Absent that, I think we probably shouldn't enable them (beyond an experiment).
>
> How about capsule the loading functionalities to a function, and call it from clangd if experimental feature is checked? In this way nothing changed if the clangd's experimental feature is off.

Yeah, a clangd flag to allow switching this on/off at without rebuilding could definitely be an option.



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3905
+    std::string Error;
+    if (llvm::sys::DynamicLibrary::LoadLibraryPermanently(Path.c_str(), &Error))
+      Diags.Report(diag::err_fe_unable_to_load_plugin) << Path << Error;
----------------
psionic12 wrote:
> sammccall wrote:
> > is this threadsafe on all platforms?
> While I can't tell, clang driver use this for a while and seems no problem, could you help to point out what's the worst case your concerned about?
clang-driver isn't multithreaded, so wouldn't show up problems here.

I'm concerned that if thread A loads plugin X, thread B loads plugin X, and thread C loads plugin Y, all concurrently and using this code path, whether they will all load correctly.

In clangd these threads could correspond to two open files and a background-index worker.

(I don't know much about dynamic loading, this may be totally fine)


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3914
+    if (P->getActionType() == PluginASTAction::ReplaceAction) {
+      Res.getFrontendOpts().ProgramAction = clang::frontend::PluginAction;
+      Res.getFrontendOpts().ActionName = Plugin.getName().str();
----------------
psionic12 wrote:
> sammccall wrote:
> > we can't support replacement of the main action in clangd. What's the plan there - ignore the plugin?
> Could you help to explain why action replacements are not supported? 
> 
> As far as I know, replacement of actions is related with Actions, which does not have directly relationship with clangd,
Clangd uses some custom FrontendActions (different ones for different ways we're parsing the file) to implement its features (e.g. track which parts of the AST are part of the main-file without deserializing the preamble, and to do indexing).
If you replace these actions, normal features will not work.

e.g.:
- https://github.com/llvm/llvm-project/blob/4df8efce80e373dd1e05bd4910c796a0c91383e7/clang-tools-extra/clangd/ParsedAST.cpp#L97
- https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/index/IndexAction.cpp#L206

If we replace these with the plugin's action, clangd won't work 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155