View Issue Details

IDProjectCategoryView StatusLast Update
0001291OpenMPTPlugins / VSTpublic2021-01-10 18:30
ReporterSaga Musix Assigned Tomanx  
PrioritynormalSeverityfeatureReproducibilityhave not tried
Status resolvedResolutionfixed 
Target VersionOpenMPT 1.30 / libopenmpt 0.6 (goals)Fixed in VersionOpenMPT 1.30 / libopenmpt 0.6 (goals) 
Summary0001291: Separate plugin bridge versions for legacy / buggy plugins
Description

We have observed many plugins which do not (any longer) work in OpenMPT, also not if run in the plugin bridge. Sometimes this is the result of using safer defaults for linker flags, OS compatibility declarations etc.

For example:

  • Many old plugins don't like Data Execution Prevention (DEP) being turned on.
  • Synth1 x64 has problems with Windows 8+ High Entropy Heap. If this flag is disabled, heap allocated addresses are more likely to stay below the 32-bit border, preventing the crash from happening in most situations.
  • Free RS-MET plugin editors crash if OpenMPT declares to be compatible with Windows 10.
  • DPI-awareness of old plugins.

To keep these old plugins alive, we could provide a second "unsafe" set of plugin bridge executables which disables DEP, high entropy heap and does not declare any operating system compatibility. DPI-awareness doesn't need to be handled this way as it can be configured at runtime.

TagsNo tags attached.
Has the bug occurred in previous versions?
Tested code revision (in case you know it)

Relationships

related to 0001290 closed Crash files from when opening Channel Matrix 2x2 plugin window 
related to 0001012 closed VST SignalAnalyzer causes crash when opened 
related to 0001030 resolvedmanx VSTi Synth1 64bit crashes / not working 
related to 0001401 resolvedmanx Do not silently ignore crashes in VST plugins by default any more 
related to 0001402 resolvedmanx Use modern process settings for the main OpenMPT executable 

Activities

manx

manx

2020-06-11 14:34

administrator   ~0004375

  • [New] PluginBridge: Split into Legacy and non-Legacy version. The legacy version disables data execution prevention (for a86 and amd64 (it is unsupported on arm)), high entropy ASLR (for 64bit architectures), and large address awareness (for 32bit architectures).
  • [Mod] PluginBridge: Enable all these fetures for the regular bridge.
  • [Reg] Also enable all these features for OpenMPT.exe itself. This will break broken plugins, which now need to be bridged.
  • [Mod] Store modern/legacy bridge flag in plugin cache. For already stored plugins, legacy is assumed in order to maximize compatibility. Newly created plugins default to modern.
  • [Imp] Plugin Dialog: Add checkbox for compatibility (legacy) bridge.
manx

manx

2020-06-11 17:20

administrator   ~0004379

manx

manx

2020-06-13 16:38

administrator   ~0004384

manx

manx

2020-07-29 11:00

administrator   ~0004410

manx

manx

2020-12-30 11:03

administrator   ~0004586

Committed as 1.30.00.24 (13999).

manx

manx

2020-12-30 11:04

administrator   ~0004589

Forum thread: https://forum.openmpt.org/index.php?topic=6537.0

jmkz

jmkz

2020-12-30 21:27

reporter   ~0004591

As a suggestion, please add an option to register plugins using legacy plugin bridge. I used DeltaSP's plugins wich were broken with the newer security implementations.
https://drive.google.com/file/d/1RtJbkFFV23YVCM44fOxkvYJTA13kvXPw/view?usp=sharing

Gerirish

Gerirish

2020-12-30 22:27

reporter   ~0004592

jmkz - have you read the "Legacy Plugin Bridge" section in https://forum.openmpt.org/index.php?topic=6537.0 ? I think that answers your question.

Saga Musix

Saga Musix

2020-12-30 22:28

administrator   ~0004593

There was no question, jmkz has a very valid point - if newly plugins are always scanned using the modern bridge variant, older plugins that crash in that variant cannot be loaded. If the modern bridge fails while scanning the plugin, OpenMPT must fall back to scanning the plugin using the legacy bridge.

Gerirish

Gerirish

2020-12-30 22:31

reporter   ~0004594

The last sentence says: There is a new setting in the Plugin Dialog that allows switching. (since 1.30.00.25).

Means it's possible to switch between legacy & new Plugin Bridge, or? Would that not answer his point?

Saga Musix

Saga Musix

2020-12-30 22:32

administrator   ~0004595

The problem is that this is encourages the user to just always enable the legacy bridge, since it will work with more plugins. This is not the same as only falling back to the legacy bridge if the modern bridge crashed during initialization.

Gerirish

Gerirish

2020-12-30 22:33

reporter   ~0004596

Thanks, got it.

manx

manx

2020-12-31 07:01

administrator   ~0004597

Agreed, this must be addressed.

manx

manx

2020-12-31 10:02

administrator   ~0004598

pluginbridge-fallback-to-legacy-v2.patch (5,724 bytes)   
Index: mptrack/Vstplug.cpp
===================================================================
--- mptrack/Vstplug.cpp	(revision 14002)
+++ mptrack/Vstplug.cpp	(working copy)
@@ -94,7 +94,7 @@
 }
 
 
-AEffect *CVstPlugin::LoadPlugin(bool maskCrashes, VSTPluginLib &plugin, HMODULE &library, bool forceBridge)
+AEffect *CVstPlugin::LoadPlugin(bool maskCrashes, VSTPluginLib &plugin, HMODULE &library, bool forceBridge, bool forceLegacy)
 {
 	const mpt::PathString &pluginPath = plugin.dllPath;
 
@@ -106,7 +106,7 @@
 	{
 		try
 		{
-			effect = BridgeWrapper::Create(plugin);
+			effect = BridgeWrapper::Create(plugin, forceLegacy);
 			if(effect != nullptr)
 			{
 				return effect;
Index: mptrack/Vstplug.h
===================================================================
--- mptrack/Vstplug.h	(revision 14002)
+++ mptrack/Vstplug.h	(working copy)
@@ -77,7 +77,7 @@
 	CVstPlugin(bool maskCrashes, HMODULE hLibrary, VSTPluginLib &factory, SNDMIXPLUGIN &mixPlugin, Vst::AEffect &effect, CSoundFile &sndFile);
 	~CVstPlugin();
 
-	static Vst::AEffect *LoadPlugin(bool maskCrashes, VSTPluginLib &plugin, HMODULE &library, bool forceBridge);
+	static Vst::AEffect *LoadPlugin(bool maskCrashes, VSTPluginLib &plugin, HMODULE &library, bool forceBridge, bool forceLegacy);
 
 protected:
 	void Initialize();
Index: pluginBridge/BridgeWrapper.cpp
===================================================================
--- pluginBridge/BridgeWrapper.cpp	(revision 14002)
+++ pluginBridge/BridgeWrapper.cpp	(working copy)
@@ -257,11 +257,11 @@
 
 
 // Create a plugin bridge object
-AEffect *BridgeWrapper::Create(const VSTPluginLib &plugin)
+AEffect *BridgeWrapper::Create(const VSTPluginLib &plugin, bool forceLegacy)
 {
 	BridgeWrapper *wrapper = new(std::nothrow) BridgeWrapper();
 	BridgeWrapper *sharedInstance = nullptr;
-	const Generation wantedGeneration = plugin.modernBridge ? Generation::Modern : Generation::Legacy;
+	const Generation wantedGeneration = (plugin.modernBridge && !forceLegacy) ? Generation::Modern : Generation::Legacy;
 
 	// Should we share instances?
 	if(plugin.shareBridgeInstance)
Index: pluginBridge/BridgeWrapper.h
===================================================================
--- pluginBridge/BridgeWrapper.h	(revision 14002)
+++ pluginBridge/BridgeWrapper.h	(working copy)
@@ -200,7 +200,7 @@
 	static bool IsPluginNative(const mpt::PathString &pluginPath) { return GetPluginBinaryType(pluginPath) == GetNativePluginBinaryType(); }
 	static uint64 GetFileVersion(const WCHAR *exePath);
 
-	static Vst::AEffect *Create(const VSTPluginLib &plugin);
+	static Vst::AEffect *Create(const VSTPluginLib &plugin, bool forceLegacy);
 
 protected:
 	BridgeWrapper();
Index: soundlib/plugins/PluginManager.cpp
===================================================================
--- soundlib/plugins/PluginManager.cpp	(revision 14002)
+++ soundlib/plugins/PluginManager.cpp	(working copy)
@@ -533,6 +533,7 @@
 	}
 
 #ifndef NO_VST
+	bool requiresLegacyBridge = false;
 	unsigned long exception = 0;
 	// Always scan plugins in a separate process
 	HINSTANCE hLib = NULL;
@@ -542,7 +543,9 @@
 		ExceptionHandler::ContextSetter ectxguard{&ectx};
 #endif // MODPLUG_TRACKER
 
-		Vst::AEffect *pEffect = CVstPlugin::LoadPlugin(maskCrashes, *plug, hLib, true);
+		Vst::AEffect *pEffect = nullptr;
+		
+		pEffect = CVstPlugin::LoadPlugin(maskCrashes, *plug, hLib, true, false);
 
 		if(pEffect != nullptr && pEffect->magic == Vst::kEffectMagic && pEffect->dispatcher != nullptr)
 		{
@@ -568,6 +571,42 @@
 			validPlug = true;
 		}
 
+		if(!validPlug || (exception != 0))
+		{
+			validPlug = false;
+			exception = 0;
+			pEffect = nullptr;
+
+			requiresLegacyBridge = true;
+
+			pEffect = CVstPlugin::LoadPlugin(maskCrashes, *plug, hLib, true, true);
+
+			if(pEffect != nullptr && pEffect->magic == Vst::kEffectMagic && pEffect->dispatcher != nullptr)
+			{
+				CVstPlugin::DispatchSEH(maskCrashes, pEffect, Vst::effOpen, 0, 0, 0, 0, exception);
+
+				plug->pluginId1 = pEffect->magic;
+				plug->pluginId2 = pEffect->uniqueID;
+
+				GetPluginInformation(maskCrashes, pEffect, *plug);
+
+#ifdef VST_LOG
+				intptr_t nver = CVstPlugin::DispatchSEH(maskCrashes, pEffect, Vst::effGetVstVersion, 0,0, nullptr, 0, exception);
+				if (!nver) nver = pEffect->version;
+				MPT_LOG(LogDebug, "VST", MPT_UFORMAT("{}: v{}.0, {} in, {} out, {} programs, {} params, flags=0x{} realQ={} offQ={}")(
+					plug->libraryName, nver,
+					pEffect->numInputs, pEffect->numOutputs,
+					mpt::ufmt::dec0<2>(pEffect->numPrograms), mpt::ufmt::dec0<2>(pEffect->numParams),
+					mpt::ufmt::HEX0<4>(static_cast<int32>(pEffect->flags)), pEffect->realQualities, pEffect->offQualities));
+#endif // VST_LOG
+
+				CVstPlugin::DispatchSEH(maskCrashes, pEffect, Vst::effClose, 0, 0, 0, 0, exception);
+
+				validPlug = true;
+			}
+
+		}
+
 	}
 	FreeLibrary(hLib);
 	if(exception != 0)
@@ -574,6 +613,13 @@
 	{
 		CVstPluginManager::ReportPlugException(MPT_UFORMAT("Exception {} while trying to load plugin \"{}\"!\n")(mpt::ufmt::HEX0<8>(exception), plug->libraryName));
 	}
+
+	if(requiresLegacyBridge)
+	{
+		plug->useBridge = true;
+		plug->modernBridge = false;
+	}
+
 #endif // NO_VST
 
 	// Now it should be safe to assume that this plugin loaded properly. :)
@@ -720,7 +766,7 @@
 		HINSTANCE hLibrary = nullptr;
 		bool validPlugin = false;
 
-		pEffect = CVstPlugin::LoadPlugin(maskCrashes, *pFound, hLibrary, TrackerSettings::Instance().bridgeAllPlugins);
+		pEffect = CVstPlugin::LoadPlugin(maskCrashes, *pFound, hLibrary, TrackerSettings::Instance().bridgeAllPlugins, false);
 
 		if(pEffect != nullptr && pEffect->dispatcher != nullptr && pEffect->magic == Vst::kEffectMagic)
 		{
manx

manx

2020-12-31 10:03

administrator   ~0004599

Maybe something like that?

Saga Musix

Saga Musix

2021-01-02 14:39

administrator   ~0004601

Makes sense, but I have not mention that it won't fix those plugins. They were already broken in earlier OpenMPT versions (even those which didn't use modern security features or declared compatibility with Windows 10), so it's likely that the plugins simply doesn't like running on Windows 10 at all.

Still, we should of course apply this patch. However, before applying the duplicated logic in PluginManager.cpp should be de-duplicated into a function.

manx

manx

2021-01-03 08:03

administrator   ~0004604

Still, we should of course apply this patch. However, before applying the duplicated logic in PluginManager.cpp should be de-duplicated into a function.

Done.

Legacy fallback when adding a plugin has been committed as r14028.

jmkz

jmkz

2021-01-04 08:05

reporter   ~0004608

I just tested, and it struggles to catch error with these DeltaSP plugins: https://drive.google.com/file/d/1RtJbkFFV23YVCM44fOxkvYJTA13kvXPw/view?usp=sharing keeps crashing OpenMPT.

Saga Musix

Saga Musix

2021-01-04 11:45

administrator   ~0004609

Yes, but they do that already in any previous OpenMPT version I have tested. Is that not the case for you?

jmkz

jmkz

2021-01-04 21:09

reporter   ~0004610

Same happens for me, more or less a year ago (something I remind is broken when switched from W 10 1909 to 2004). Module in question was last saved using 1.29.00.53. So maybe the last working version.

Saga Musix

Saga Musix

2021-01-04 21:10

administrator   ~0004611

As said, I tried version up to OpenMPT 1.22 and they all showed the same issue. It's probably a Windows update that broke it.

Issue History

Date Modified Username Field Change
2020-01-12 14:43 Saga Musix New Issue
2020-01-12 14:44 Saga Musix Relationship added related to 0001290
2020-01-12 14:44 Saga Musix Relationship added related to 0001012
2020-01-12 14:45 Saga Musix Relationship added related to 0001030
2020-06-11 12:37 manx Assigned To => manx
2020-06-11 12:37 manx Status new => acknowledged
2020-06-11 12:37 manx Target Version => OpenMPT 1.30 / libopenmpt 0.6 (goals)
2020-06-11 14:34 manx Note Added: 0004375
2020-06-11 14:34 manx File Added: pluginbridge-legacy-v1.patch.7z
2020-06-11 14:34 manx Status acknowledged => feedback
2020-06-11 17:20 manx Note Added: 0004379
2020-06-11 17:20 manx File Added: pluginbridge-legacy-v2.patch.7z
2020-06-13 16:38 manx Note Added: 0004384
2020-06-13 16:38 manx File Added: pluginbridge-legacy-v3.patch.7z
2020-07-29 11:00 manx Note Added: 0004410
2020-07-29 11:00 manx File Added: pluginbridge-legacy-v4.7z
2020-12-22 17:04 manx Relationship added related to 0001401
2020-12-23 08:23 manx Relationship added related to 0001402
2020-12-30 11:03 manx Note Added: 0004586
2020-12-30 11:04 manx Note Added: 0004589
2020-12-30 21:27 jmkz Note Added: 0004591
2020-12-30 22:27 Gerirish Note Added: 0004592
2020-12-30 22:28 Saga Musix Note Added: 0004593
2020-12-30 22:28 Saga Musix Status feedback => assigned
2020-12-30 22:31 Gerirish Note Added: 0004594
2020-12-30 22:32 Saga Musix Note Added: 0004595
2020-12-30 22:33 Gerirish Note Added: 0004596
2020-12-31 07:01 manx Status assigned => confirmed
2020-12-31 07:01 manx Note Added: 0004597
2020-12-31 10:02 manx Note Added: 0004598
2020-12-31 10:02 manx File Added: pluginbridge-fallback-to-legacy-v2.patch
2020-12-31 10:03 manx Assigned To manx => Saga Musix
2020-12-31 10:03 manx Status confirmed => feedback
2020-12-31 10:03 manx Note Added: 0004599
2021-01-02 14:39 Saga Musix Note Added: 0004601
2021-01-03 08:03 manx Note Added: 0004604
2021-01-03 08:04 manx Assigned To Saga Musix => manx
2021-01-04 08:05 jmkz Note Added: 0004608
2021-01-04 11:45 Saga Musix Note Added: 0004609
2021-01-04 11:45 Saga Musix Status feedback => assigned
2021-01-04 21:09 jmkz Note Added: 0004610
2021-01-04 21:10 Saga Musix Note Added: 0004611
2021-01-10 18:28 manx Status assigned => resolved
2021-01-10 18:28 manx Resolution open => fixed
2021-01-10 18:28 manx Fixed in Version => OpenMPT 1.30 / libopenmpt 0.6 (goals)