sammccall added a comment.
In D92155#2419346 <https://reviews.llvm.org/D92155#2419346>, @psionic12 wrote:
>> 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)
>
> Yes I know how clang manager files through vfs, it just that loading
> libraries does not involve vfs at all, the path of a lib is passed directly
> to the system level API (eg, dlopen on Linux, System::Load on Windows), so I
> still can't understand what you are concerning,
This is exactly the problem, filenames specified in *clang* flags are
*supposed* to be read from the VFS. (In practice this probably just means we'd
need to disable this feature in environments where VFS is used)
> Or, could you help to point out what's the difference between passing a
> plugin path through *clangd* startup command line and through clang flags?
Sure. TL;DR is: clangd flags are configured by the user, user can be fully
responsible for security/stability.
clang flags are configured by the project. If they're bad, we can e.g. give bad
diagnostics, but can't crash or compromise security.
More detail:
In the simplest possible case, clangd is configured as follows:
1. user downloads clangd binary
2. user installs an LSP plugin for their editor, and configures the plugin to
use /usr/bin/clangd for C++ files. clangd starts when the editor does
3. the build system for $PROJECT generates $PROJECT/compile_commands.json
4. when the user opens $PROJECT/src/foo.cpp in the editor, it notifies clangd.
clangd searches for $PROJECT/compile_commands.json, finds the clang arguments,
and uses them to parse foo.cpp
*clangd* command-line flags would be added explicitly by the user at step 2. We
can reasonably ask the user to be aware/responsible for security/stability
implications of doing this, including with their particular clangd version. We
can also ask them to run `clangd --check` without the plugin flag to test
whether the plugin is causing a stability problem.
*clang* command-line flags are added implicitly in step 3. Or they could simply
be checked into the repository - nothing ensures they were generated locally by
the build system. The point is in typical usage they are not controlled by the
user directly, and from a security perspective are not trusted (as safely
opening files from untrusted repos is a reasonable expectation). So if we're
loading plugins based on instructions in clang command-line flags, clangd bears
most of the responsibility for making sure that's safe and correct (and I don't
see a way to do that).
================
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:
> > 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)
> In this case I can make sure multiple thread works just fine with
> `LoadLibraryPermanently`, I checked all the dynamic loading API on most
> platforms and all of the are thread-safe(it's rare to see system APIs which
> are not thread safe, to me).
That sounds good! I do see mutexes in the posix implementation so I guess the
LLVM wrapper is intended to be threadsafe.
================
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:
> > 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
> I think this is the part I didn't expected, seems a standalone action loading
> logic needs to exist.
>
> I'll try to come up a patch which has standalone plugin loading and guard it
> with experimental checking.
It seems plugins can run before/after/replace the main action.
I wonder if it'd break things to silently "promote" a replace plugin to e.g. an
after one, which I guess would solve this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92155/new/
https://reviews.llvm.org/D92155
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
