Skip to content

Fix Heap Alloction issues in QTFred#7334

Merged
wookieejedi merged 2 commits into
scp-fs2open:masterfrom
TheForce172:fix/QTFreHeapCrashes
May 1, 2026
Merged

Fix Heap Alloction issues in QTFred#7334
wookieejedi merged 2 commits into
scp-fs2open:masterfrom
TheForce172:fix/QTFreHeapCrashes

Conversation

@TheForce172

@TheForce172 TheForce172 commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

There have been multiple heap crash issues with QTFred on that have been traced to the fact that QT Distributes its libraries with dynamic linking while we are using static linking. We most likely can't distribute statically linked version of QT due to licencing issues between QT and FSO's licenses. So are only option is to compile dynamically if we are building QTFRED.

Fixes Multiple QTFred heap crashes due to mismatched runtime types.
@wookieejedi wookieejedi added fix A fix for bugs, not-a-bugs, and/or regressions. qtfred A feature or issue related to qtFred. labels Mar 30, 2026
@github-project-automation github-project-automation Bot moved this to Work In Progress (PRs) in qtFRED2 Mar 30, 2026
@wookieejedi wookieejedi added this to the Release 26.0 milestone Mar 30, 2026

@JohnAFernandez JohnAFernandez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anyone else is going to look at this. Hold on to your butts.

@github-project-automation github-project-automation Bot moved this from Work In Progress (PRs) to In Review (PRs) in qtFRED2 Apr 3, 2026
@wookieejedi

Copy link
Copy Markdown
Member

Given Cyborg approved and TheE says it looks fine, I'll plan to merge tomorrow.

@Goober5000 Goober5000 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does look like the best solution, from a design standpoint, but this PR only touches MSVC. We'll need to build QtFRED dynamically regardless of whatever build environment we use or what OS we build on.

@notimaginative

Copy link
Copy Markdown
Contributor

This does look like the best solution, from a design standpoint, but this PR only touches MSVC. We'll need to build QtFRED dynamically regardless of whatever build environment we use or what OS we build on.

I'm pretty sure this is an issue specific to MSVC (or using that toolchain). So this shouldn't be a problem on other platforms and/or toolchains. I'm not sure whether it applies to cross-compilation though (don't think so?).

@Goober5000

Copy link
Copy Markdown
Contributor

This does look like the best solution, from a design standpoint, but this PR only touches MSVC. We'll need to build QtFRED dynamically regardless of whatever build environment we use or what OS we build on.

I'm pretty sure this is an issue specific to MSVC (or using that toolchain). So this shouldn't be a problem on other platforms and/or toolchains. I'm not sure whether it applies to cross-compilation though (don't think so?).

But licensing issues would apply regardless of platform. Or do the other platforms already link dynamically?

@notimaginative

notimaginative commented Apr 4, 2026

Copy link
Copy Markdown
Contributor

This does look like the best solution, from a design standpoint, but this PR only touches MSVC. We'll need to build QtFRED dynamically regardless of whatever build environment we use or what OS we build on.

I'm pretty sure this is an issue specific to MSVC (or using that toolchain). So this shouldn't be a problem on other platforms and/or toolchains. I'm not sure whether it applies to cross-compilation though (don't think so?).

But licensing issues would apply regardless of platform. Or do the other platforms already link dynamically?

Oh no, this isn't about linking with Qt, it's about how linking is done with the Windows runtime. Qt links the Windows runtime dynamically, while FSO links it statically. And you can't safely mix the two methods. And it's much easier to just change how FSO links than Qt does.

@Goober5000

Goober5000 commented Apr 4, 2026

Copy link
Copy Markdown
Contributor

Ok, fair enough.

EDIT: A quick search shows that MSVC_USE_RUNTIME_DLL is used in several places across the codebase, including to determine whether MFC itself is linked dynamically. I'm wondering if MSVC_USE_RUNTIME_DLL should be set based on FSO_BUILD_QTFRED, rather than adding this one special case here.

@MjnMixael

Copy link
Copy Markdown
Contributor

What needs to happen to move this one forward?

@wookieejedi

wookieejedi commented Apr 13, 2026

Copy link
Copy Markdown
Member

KNet associated PR has been merged, so we can plan to merge this once the next KNet version is out. Per Goober's inquiry about if MSVC_USE_RUNTIME_DLL should be set based on FSO_BUILD_QTFRED on Discord, taylor stated the following in response to Personally, no, I don't think it should do that. Primarily since FSO would use a different runtime depending on whether FSO_BUILD_QTFRED was enabled and might affect debugging and ability to reproduce bugs. But I'm not up on Windows internals enough to confidently say it would be a problem

@wookieejedi

Copy link
Copy Markdown
Member

Update, next KNet version is planned to come out in about two weeks, so will plan to merge this PR when that happens as noted above.

@wookieejedi wookieejedi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given discussion here and on Discord, approving and will also dismiss stale reviews and plan to merge once KNet 1.3.5 releases in the near future.

@Goober5000

Copy link
Copy Markdown
Contributor

I had lost track of this PR, so it was good to get the ping. Given Taylor's response, I'm happy to approve it.

@wookieejedi

wookieejedi commented May 1, 2026

Copy link
Copy Markdown
Member

KNet 1.3.5 is out so will merge this later today, thanks all!

@wookieejedi wookieejedi merged commit 0e43b19 into scp-fs2open:master May 1, 2026
20 checks passed
@github-project-automation github-project-automation Bot moved this from In Review (PRs) to Done in qtFRED2 May 1, 2026
Goober5000 added a commit that referenced this pull request May 8, 2026
PR #7334 caused an unintended side effect when building both FRED and QtFRED.  With QtFRED enabled, FRED2 sources were compiled with _AFXDLL defined while still linking against static MFC, causing a crash on startup.  To fix this, keep the conditionals in sync between cmake/toolchain-msvc.cmake and fred2/CMakeLists.txt.

Also, do not explicitly add static MFC libraries if we are using dynamic linking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix A fix for bugs, not-a-bugs, and/or regressions. qtfred A feature or issue related to qtFred.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants