Support for Plugin subfolder DLL search path

[MattR]

I just created a pull request to enhance the plugin DLL search behavior in orbiter. My project, Skybolt, needs to load many DLLs for OpenSceneGraph, glew etc. Currently Orbiter searches for DLLs in the Modules, and Modules/Plugins folders. Rather than pollute these folders with my project-specific plugins, I'd like Orbiter to load DLLs from my project's subfolder: Modules/Plugins/OrbiterSkyboltClient. The pull request adds support for this behavior. If a directory exists with the same name as a plugin, it is added to the PATH and searched by LoadLibrary().

Does this sound like a good solution?

 

[Urwumpe]

I think it would be better, if this is done explicitly by maintaining a list of (additional) directories to search for plugins in Orbiter.cfg

Even if we go by using a one-click install mechanism one day for add-ons. I think such wide-ranging implicit behaviour could otherwise create an unnecessary security risk. It doesn't even need to be a folder named by the add-on itself, if done automatically (Lets say, an misbehaving vessel installs some weird stuff into the subfolder "transx" to hide its purpose).

Of course, using C++ modules in first place is also a security risk there... but I wouldn't want to replace them all. It would just be better if more advanced stuff could be done by Lua and other scripting languages.

 

[MattR]

I take your point about security. How about this alternative solution: have an optional MyPlugin.cfg beside MyPlugin.dll which defines search paths? The advantage of that approach is the user doesn't need to manually edit config files, and the dependencies are explicitly defined. That said, a malicious plugin could add a transx.cfg to inject unwanted DLLs, but that doesn't seem any less secure than a malicious plugin installer adding paths to the user's Orbiter.cfg

 

[Face]

In addition to what Urwumpe said, I can see version problems coming up. If you have addon A using a library C, which also addon B is using, but both use a different version of it, chances are that the user encounters various type/interface mismatches depending on what addons he has installed. The chance gets greater the more addons are installed. If somebody comes up with something like CVEL as implicitly linkable DLL, users would be in for a great run-time pain if everybody just dumps their CVEL.dll into its own folder.
Of course this problem is also there in the common folder architecture we have right now, but it is immediately obvious on install due to overwritten files.
AFAIK, C++ does not have a strong naming system like .NET . A mandatory file naming scheme could compensate a little for that, although it would be cumbersome and still error-prone. Another option is using explicit linking (or late-binding) instead of implicit linking, but this often needs an architecture redesign of the addon/library interface.

The pull request adds support for this behavior. If a directory exists with the same name as a plugin, it is added to the PATH and searched by LoadLibrary().

If your addon already uses explicit linking via LoadLibrary calls, why not give the absolute path (to the subdir) instead of just the module name? Is it due to subsequent implicit linking in the libs you are using? In this case you could use LoadLibraryEx with absolute path and LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR .

 

[MattR]

If your addon already uses explicit linking via LoadLibrary calls, why not give the absolute path (to the subdir) instead of just the module name? Is it due to subsequent implicit linking in the libs you are using? In this case you could use LoadLibraryEx with absolute path and LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR .

My plugin DLL is implicitly linked to the dependency DLLs, so there are no LoadLibrary calls involved (except for the one Orbiter uses to load my plugin). I don't think explicit linking will work without changing the code of the dependencies since those dependencies in turn have DLL dependencies. Some other solutions are:

1. Static link everything inside my plugin. This may be the safest option for avoiding versioning and security issues.
2. Delay-load dependencies and redirect DLL paths to the subfolder. This is doable, but messy.
3. Define DLL paths using manifests. This should be a clean solution if I can get it to work, but I hit a roadblock with nested dependencies which I have asked about on stack overflow.

 

[Face]

I don't think explicit linking will work without changing the code of the dependencies since those dependencies in turn have DLL dependencies.

That's why I suggested using LoadLibraryEx() in the first explicit linking. The subsequent DLL dependencies would then load from the initial DLL load directory.

 

[MattR]

That's why I suggested using LoadLibraryEx() in the first explicit linking. The subsequent DLL dependencies would then load from the initial DLL load directory.

Ah I see, sorry I misunderstood. Yes that's a good suggestion, but I even changing just the top level module to use explicit linking will require a lot of work and introduce complexity. I think I'll go with static linking since my build infrastructure supports it trivially (using conan), and no code changes are needed.

 

[Face]

Your proposal got me thinking: what about introducing a second load flow in Orbiter based on LoadLibraryEx? I.e. instead of referencing files, reference directories. That would be easy enough for vessels, since they have a config file, but it could also be done for plugins, Orbiter just needs to enumerate into subdirs as well. It could be even done in a backwards-compatible way, so legacy vessels and modules are loaded the old way, but new vessels and modules use the encapsulation method.

 

[MattR]

Your proposal got me thinking: what about introducing a second load flow in Orbiter based on LoadLibraryEx? I.e. instead of referencing files, reference directories. That would be easy enough for vessels, since they have a config file, but it could also be done for plugins, Orbiter just needs to enumerate into subdirs as well. It could be even done in a backwards-compatible way, so legacy vessels and modules are loaded the old way, but new vessels and modules use the encapsulation method.

So, if I understand what you're saying, define a plugin as a folder containing dlls (and potentially other assets and a cfg file). The way I imagine it working is Orbiter scans for plugin folders and loads myplugin/myplugin.dll with LoadLibraryEx(... LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR). That would solve my problem nicely, and avoid introducing ambiguity with dll clashes. Does that sound right?

 

[Face]

So, if I understand what you're saying, define a plugin as a folder containing dlls (and potentially other assets and a cfg file). The way I imagine it working is Orbiter scans for plugin folders and loads myplugin/myplugin.dll with LoadLibraryEx(... LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR). That would solve my problem nicely, and avoid introducing ambiguity with dll clashes. Does that sound right?

Yep. That's the idea. If LoadLibraryEx() works as described in the MSDN, that should be a cool solution.

Quick&dirty example for OVP client enumeration: https://github.com/Face-1/orbiter/commit/0d4acde704dbbc434a7a376e6dc0d0119a29aacd

 

[MattR]

I tested your example, fixed a few issues and cleaned it up a bit. It works perfectly for my use case. If everyone is happy can we get this feature merged?

Orbiter searches for modules within an optional module subfolder · matthew-reid/orbiter@26b5a58

Open-source repository of Orbiter Space Flight Simulator - Orbiter searches for modules within an optional module subfolder · matthew-reid/orbiter@26b5a58

github.com

 

[Face]

If everyone is happy can we get this feature merged?

Your best bet to get something merged is probably contacting one of the current members of the orbitersim organization, like depicted in this post: https://www.orbiter-forum.com/threa...ide-open-source-development.40409/post-593058

The current solution is only implementing the alternative load flow for OVP clients, though. If the concept holds, it would be good to have that for modules and vessels as well.