Support for Plugin subfolder DLL search path

MattR

Active member
Joined
Aug 5, 2021
Messages
42
Reaction score
124
Points
33
Location
Melbourne
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

Not funny anymore
Addon Developer
Donator
Joined
Feb 6, 2008
Messages
37,675
Reaction score
2,406
Points
203
Location
Wolfsburg
Preferred Pronouns
Sire
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

Active member
Joined
Aug 5, 2021
Messages
42
Reaction score
124
Points
33
Location
Melbourne
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

Well-known member
Orbiter Contributor
Addon Developer
Beta Tester
Joined
Mar 18, 2008
Messages
4,404
Reaction score
581
Points
153
Location
Vienna
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 .
 
Last edited:

MattR

Active member
Joined
Aug 5, 2021
Messages
42
Reaction score
124
Points
33
Location
Melbourne
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

Well-known member
Orbiter Contributor
Addon Developer
Beta Tester
Joined
Mar 18, 2008
Messages
4,404
Reaction score
581
Points
153
Location
Vienna
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

Active member
Joined
Aug 5, 2021
Messages
42
Reaction score
124
Points
33
Location
Melbourne
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

Well-known member
Orbiter Contributor
Addon Developer
Beta Tester
Joined
Mar 18, 2008
Messages
4,404
Reaction score
581
Points
153
Location
Vienna
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

Active member
Joined
Aug 5, 2021
Messages
42
Reaction score
124
Points
33
Location
Melbourne
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

Well-known member
Orbiter Contributor
Addon Developer
Beta Tester
Joined
Mar 18, 2008
Messages
4,404
Reaction score
581
Points
153
Location
Vienna
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
 
Last edited:

MattR

Active member
Joined
Aug 5, 2021
Messages
42
Reaction score
124
Points
33
Location
Melbourne

Face

Well-known member
Orbiter Contributor
Addon Developer
Beta Tester
Joined
Mar 18, 2008
Messages
4,404
Reaction score
581
Points
153
Location
Vienna
Top