Fix VST crashes on Linux

[Fastigium]

Disables ASLR when building RemoteVstPlugin for Windows VSTs on Linux. See #7830.

If we end up going for this method for real, the patched winebuild repo (https://github.com/Fastigium/winebuild) should probably be moved to the LMMS organization 🙂

[messmerd]

LGTM

[tresf]

should probably be moved to the LMMS organization 🙂

I'm perfectly fine creating the winebuild fork over at lmms/winebuild. This is small enough that it shouldn't significantly impact the build times of platforms that don't need it (Windows, macOS).

Since this change may be needed upstream, I've started the conversation with the winehq devel community here: https://list.winehq.org/mailman3/hyperkitty/list/wine-devel@winehq.org/thread/AAPOGZN3H732GXNKB4ZV7TRNCRIBM5AG/.

I think @Fastigium's instinct to keep this as a draft until we have more direction from upstream is a good idea. With regards to using a small cloned portion of the wine project for this, this is a bit non-standard as it prevents our mirror from being synced with upstream (but the alternative is to mirror a huge repo, which we also don't want 😅), so we'll have to weigh the two options. Even if winehq adds this feature, it won't be immediately added to the winehq-stable branch, so we'll have to determine what's simplest for our CI build and our local dev builds (which will most likely be a one-off fork as @Fastigium has created).

I did look into other git strategies (such as git subtree, git sparse-checkout) but couldn't find any that would reduce complexity over a one-off fork, so lmms/winebuild will likely become the temporary home for this patch moving forward.

[tresf]

Linking #7976

[tresf]

Upstream bug report filed (per advice of winehq mailing list) https://bugs.winehq.org/show_bug.cgi?id=58480

[messmerd]

We could fetch the Git repo in the ExternalProject_Add function, which would remove the need for a git submodule. That way Windows, macOS, and non-Wine Linux builds won't download the patched winebuild that they won't ever use.

[tresf]

I'm going to push some experimental changes in an attempt to move @Fastigium's patch to a dedicated wine fork. I expect it to temporarily break builds (sorry) but it should be rather trivial to fix.

My fork is temporary... once we have this working, we should move it from my own personal fork to a lmms-managed gitlab fork that can be used for create a merge request into upstream wine.

@messmerd I'll try to fix any issues by reviewing the CI logs but I'd appreciate any assistance fixing the ExternalProject_Add.

[tresf]

The custom version of winebuild has been successfully moved to a github fork. Attempts to get just-the-right settings for ExternalProject_Add were tricky, but I think it's in good shape. Note, the clone and build will occur in CMAKE_CURRENT_BINARY_DIR (versus previous CMAKE_CURRENT_LIST_DIR). This avoids having to touch the .gitignore.

Before this is merged, I would like to have an upstream MR against winehq official and I'd like the GIT_REPO to point to a gitlab (NOT github) fork so that there's an actionable item associated with removing this patch some time long into the future.

[tresf]

lmms/winebuild will likely become the temporary home for this patch moving forward.

I've been approved to create a fork over at winehq (thanks Alexandre Julliard), so this new fork is arguably a more contextual home, albeit less organizational (since other LMMS maintainers won't have access to my fork). This is because winehq hosts their own Gitlab instance.

This is a lot of words to say, if upstream is receptive, we can point to upstream once merged. If not, we'll have to determine a permanent home for our patches (which -- for simplicity and organization reasons -- probably would remain in github).

Upstream PR: https://gitlab.winehq.org/wine/wine/-/merge_requests/8786

[tresf]

After scratching my head a bit on Discord and second-guessing the above patch, I came up with the following question...

@tresf wrote:

So just a thought.... but if DYNAMICBASE is enabled for 32-bit binaries and they DO NOT suffer this crash, doesn't that mean that HIGHENTROPYVA was the culprit all along?

Just thonking on this reading my own PR over and over looking for areas of scrutiny.

@messmerd wrote:

[I've tested] it with just IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA disabled and IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE left as-is to see if it the bug exists in the 64-bit VSTs. With high-entropy-VA disabled, and dynamicbase unmodified from the upstream winebuild, the 64-bit Synth1 VST did not crash.

Link to convo: https://discord.com/channels/203559236729438208/1383830615429877851/1407401026847506502

... this suggests that we may be safe leaving DYNAMICBASE enabled. If this is true, we'll need to partially revert #7976 and add a dedicated high-entropy flag to wine. I've switched the wine patch to draft until we know more.

[tresf]

Changes:

• Merged in changes from master
• Switched PR to ONLY set HIGHTENTROPYVA flag per discussion above/discord/upstream. ASLR is re-enabled with these latest commits.

Despite the name of this PR to indicate "Linux", recent tests suggest that the Windows PR #7976 may have been overly ambitious in its compiler flags, so I've turned that back off via 4c1f989 so we'll have binaries to test against once the CI finishes.

[tresf]

Despite the name of this PR to indicate "Linux", recent tests suggest that the Windows PR #7976 may have been overly ambitious in its compiler flags, so I've turned that back off via 4c1f989 so we'll have binaries to test against once the CI finishes.

After some conversations with winehq claiming to NOT support granularity between /HIGHTENTROPYVA:NO and /DYNAMICBASE:NO, I've decided to consolidate all settings down to simply disabling DYNAMICBASE. This has the luxury of simplicity but the downside of being potentially misleading, since 32-bit VSTs were never impacted by this, yet we're going to toggle it anyways. I've explained this briefly in a code comment.

lmms/plugins/VstBase/RemoteVstPlugin/CMakeLists.txt

Lines 74 to 76 in 26f1da6

	# If we're building a Windows executable, add linker flags to disable ASLR
	# This is needed because several old VST dlls crash when high entropy ASLR is in use
	# /HIGHENTROPYVA:YES relies on /DYNAMICBASE:YES so this ensures maximum VST compatiblity

[messmerd]

I tested a4b1ea2 on Linux Mint 22 (x86_64) with Wine 10.11. No issues were encountered when using the 64-bit and 32-bit Synth1 VST.

[tresf]

Ok, we're in the home stretch here... I rebased on master, squashed up the commits into the three phases (1. Original fork, 2. ExternalProject_Add, 3. Soon-to-be-upstream).

After some back and forth, winehq -- specifically, Alexandre Julliard -- has been very receptive to our patch and I believe the patch will be merged upstream very soon since now we're working on updating the manpage for winebuild🎉! This patch will land on a future wine-devel branch, so it'll be a long time before it reaches the winehq-stable repo. We may choose to point our project at winehq-devel once it lands there by changing this line in our GitHub actions:

lmms/.github/workflows/build.yml

Line 48 in dec6a04

sudo apt-get install -y --install-recommends g++-multilib gcc-multilib winehq-stable wine-stable-dev

To reiterate why we don't just point at upstream... It's mainly to avoid building the entirety of wine in our toolchain. Instead, we're only building tools/winebuild as a standalone and to do this we must ship our own custom CMakeLists.txt file which has no home upstream, hence the fork.

In addition to the recent commits consolidation, I've also cleaned up my fork to match what I believe will be merged to upstream.

This PR can probably be merged before https://gitlab.winehq.org/wine/wine/-/merge_requests/8786 is accepted if it gets stalled for any reason. Retesting the 64-bit VSTs on Windows and Linux should be done one more time before final merge.

[tresf]

Upstream PR https://gitlab.winehq.org/wine/wine/-/merge_requests/8786 has been merged!

Retesting the 64-bit VSTs on Windows and Linux should be done one more time before final merge.

Switch to Intel to do this now, then will merge.

[tresf]

Testing results, MSVC is failing. Will baseline test against #7976 and report my findings.

OS	Build	Synth1 (32-bit)	Synth1 (64-bit)
Windows 11 x64	567dbb1 (msvc)	🚫 FAIL	🚫 FAIL
Windows 11 x64	567dbb1 (mingw)	✅ PASS	✅ PASS
Ubuntu 24.04 x64 / Wine 10.0	567dbb1	✅ PASS	✅ PASS

[tresf]

Will baseline test against #7976 and report my findings.

#7976 also fails in my testing. I'm in a VM, so I'll fire up bare metal to see if it changes anything.

[tresf]

Bare metal results added... Hmm...

OS	Build	Synth1 (32-bit)	Synth1 (64-bit)
Windows 11 x64 (vm)	567dbb1 (msvc)	🚫 FAIL	🚫 FAIL
Windows 11 x64 (bare metal)	567dbb1 (msvc)	🚫 FAIL	✅ PASS
Windows 11 x64	567dbb1 (mingw)	✅ PASS	✅ PASS
Ubuntu 24.04 x64 / Wine 10.0	567dbb1	✅ PASS	✅ PASS

When 32-bit fails on bare metal, it says "The application was unable to start correctly (0xc000007b). Click OK to close the application.". This error did not show in the VM. The results are identical both this this PR as well as with #7976, so I do not suspect any regression as a result of this PR specifically, but I think MSVC stills needs much investigation before claiming that VST plugins will work there... There's something still causing stability issues in our MSVC builds. I would request that we open a dedicated bug report for this when the others are closed.

[tresf]

I'm stumped on the MSVC regressions, but I've opened a dedicated bug report here:

• VSTs are unreliable with MSVC builds #8049

I'll merge this PR so that Linux VSTs can immediately benefit from the upstream fixes here. Huge thanks to @Fastigium for finding and fixing this ASLR-related issue with winegcc as well as it's sister-issue for mingw and MSVC in #7976.

[messmerd]

The upstream fix was released in Wine 10.14 and can now be installed through the winehq-devel .deb package.

[tresf]

The upstream fix was released in Wine 10.14 and can now be installed through the winehq-devel .deb package.

This is great news! This won't fix local builds, so I'm curious what the timeline should be on removing the no-longer-needed parts... At a glance:

Remove this entire section:

• lmms/plugins/VstBase/CMakeLists.txt

  Lines 50 to 67 in a1e3098

  	# If we're building wine-powered RemoteVstPlugins for Linux,
  	# use a custom winebuild to fix ASLR-related VST crashes.
  	# See upstream bug report: https://bugs.winehq.org/show_bug.cgi?id=58480
  	if(LMMS_BUILD_LINUX AND (LMMS_HAVE_VST_32 OR LMMS_HAVE_VST_64))
  	ExternalProject_Add(wine
  	GIT_REPOSITORY https://github.com/tresf/wine
  	GIT_TAG 1f8bb63e75baa5c9f901c8f50b4ea9dd69e0baa0
  	GIT_SHALLOW ON
  	SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/wine"
  	SOURCE_SUBDIR tools/winebuild
  	BUILD_ALWAYS ON
  	BUILD_IN_SOURCE ON
  	INSTALL_COMMAND ""
  	)
  	ExternalProject_Get_Property(wine SOURCE_DIR)
  	set(CUSTOM_WINEBUILD_EXECUTABLE "${SOURCE_DIR}/tools/winebuild/winebuild")
  	message(STATUS "Using custom winebuild: ${CUSTOM_WINEBUILD_EXECUTABLE}")
  	endif()

Remove --winebuild \"${CUSTOM_WINEBUILD_EXECUTABLE}\" from this:

• lmms/plugins/VstBase/RemoteVstPlugin32.cmake

  Line 38 in a1e3098

  "-DCMAKE_CXX_FLAGS=-m32 --winebuild \"${CUSTOM_WINEBUILD_EXECUTABLE}\" -Wb,--disable-dynamicbase"

... but this code will continue to work for local copies with other wine versions, so I'm unsure how soon we should do this. Thoughts welcome.

Opened a dedicated issue here: #8076