Hey all,
We found an issue when running our game with the stomp allocator.
The point that it tripped on is with the StructMemory of FInstancedStruct. In particular, this line is the problematic one:
if (ScriptStruct != nullptr && !ScriptStruct->HasAnyFlags(RF_BeginDestroyed | RF_FinishDestroyed | RF_MirroredGarbage))
The issue is that checking if ScriptStruct is not null, doesn’t necessarily implies that it is safe to access ScriptStruct for anything, for it could be an object that got GC’ed and the result of HasAnyFlags is now undefined.
Something we’ve done locally to work around the issue is to change ScriptStruct from being a raw ptr to being a TWeakObjectPtr which correctly handle this situation, but we’re wondering if it’s a known issue or if there’s any other recommendation around the problem.
Thank you!
Why not just use IsValid(ScriptStruct) instead of null check? It’s a UObject after all.
Seems to me that in order for the ScriptStruct to ever be marked for GC, no FInstancedStruct could exist that references it, as FInstancedStruct::AddStructReferencedObjects adds a reference to the ScriptStruct, so if you are tripping on a freed script struct, you are catching an FInstancedStruct that exists outside the visibility of the collection system.
This is something we have looked a bit into. The original change came from another thread here on EPS.
Speaking with other devs who have been working on StructUtils, the opinion is more in line with what Jeremy has posted about using UPROPERTY and reference collecting the FInstancedStruct. If you are encountering a crash on shutdown here, we may be able to better address this rather than going straight to use a weak ptr.
-James
Thanks for the extra info. I think you’re seeing an old bug that caused FInstancedStruct::Reset to run too late during object destruction.
This was fixed in CL# 32659438, but that fix isn’t present in UE 5.4 (it is in UE 5.5). If you’re able to merge that CL into your Engine then hopefully that will fix the issue you’re seeing.
Yeah, that’s the one. Your other workaround for now, if you can somehow identify which object has the FInstancedStruct, is to manually call Reset() on it during the FinishDestroy (or BeginDestroy) of the object.
> Why not just use IsValid(ScriptStruct) instead of null check?
IsValid(ScriptStruct) could still lead to a use after free if the ScriptStruct memory isn’t valid memory though, right?
> you are catching an FInstancedStruct that exists outside the visibility of the collection system.
I am not sure what you mean there, it is possible to create a FInstancedStruct out of thin air and give it a blob of memory or a UStruct, and you can create a copy of that FInstancedStruct and keep chaining it.
It doesn’t seem that there’s anything that ties the FInstancedStruct’s owned memory to anything, or prevent the pointed object from being unloaded? AddStructReferencedObjects seems to be very sparingly used in the Engine, I only see it being used in Slate views, TableViewTypeTraits.h
One more bit of detail, is that it might not be because it doesn’t play nice with the GC, but more that nothing seems to prevent a FInstancedStruct from outliving the GC and yet it accesses the memory in its dtor.
The time where the stomp allocator tripped was during shutdown, which seems similar to what seemed like a first attempt at addressing the issue in the vanilla code:
// We check if the struct is still valid, otherwise static` Instanced Structs can crash here during teardown
if (ScriptStruct != nullptr && !ScriptStruct->HasAnyFlags(RF_BeginDestroyed | RF_FinishDestroyed | RF_MirroredGarbage))`
Interesting. I’m not Epic so just tossing my 2c in.
I’m curious what the context of an FInstancedStruct is that it outlives the GC. Doesn’t that also point to it not being within a GC chain? (inside a uobject, etc). It seems like a “don’t do this” issue, and not necessarily a bug, in the same way it’s a bug not to UPROPERTY() member variables that need to be visible to the GC. FInstancedStruct members should also be UPROPERTY() for the same reason.
That’s a fair point, but I think one difference with say, a UObject, is that FInstancedStruct are “structs”, or at least that’s what it seems to imply, some sort of bonified FVector which can also reference a UObject.
You can create one out of thin air while serializing/deserializing stuff for example, using it as a way to serialize generic data coming from other parts of the project.
Here’s an example in vanilla UE where they are used in such a way (as far as I can tell):
https://github.com/EpicGames/UnrealEngine/blob/800c03c848b65daec3e343852fff5677c4c1139f/Engine/Plugins/VirtualProduction/Avalanche/Source/AvalancheTransition/Public/Behavior/AvaTransitionBehaviorInstance.h#L31
So if the FInstancedStruct has to outlive the referenced UStruct, nothing enforces that - well, apart that it’ll crash if you don’t … sometimes … because it depends of if the memory has been reclaimed or not, which is probably why it only showed up when I ran with the stomp allocator.
One thing I didn’t look into as well is if there’s any ordering implication to when the dtor for FInstancedStruct runs vs when the dtor for the UStruct runs, even if they end up being released at the same time.
Thanks for the reply!
So I went ahead and reproed the issue in our project to get the exact callstack which trips the stomp malloc.
Here it is:
[Inlined] [UnrealEditor-StructUtils.dll] UObjectBase::GetFlags() UObjectBase.h:209 [Inlined] [UnrealEditor-StructUtils.dll] UObjectBaseUtility::HasAnyFlags(EObjectFlags) UObjectBaseUtility.h:90 [UnrealEditor-StructUtils.dll] FInstancedStruct::Reset() InstancedStruct.cpp:153 [UnrealEditor-StructUtils.dll] FInstancedStruct::~FInstancedStruct() InstancedStruct.h:54 [UnrealEditor-CoreUObject.dll] FAsyncPurge::TickDestroyGameThreadObjects(bool, double, double) GarbageCollection.cpp:770 [Inlined] [UnrealEditor-CoreUObject.dll] FAsyncPurge::TickPurge(bool, double, double) GarbageCollection.cpp:893 [UnrealEditor-CoreUObject.dll] IncrementalDestroyGarbage(bool, double) GarbageCollection.cpp:4899 [UnrealEditor-CoreUObject.dll] IncrementalPurgeGarbage(bool, double) GarbageCollection.cpp:4574 [UnrealEditor-CoreUObject.dll] UE::GC::PostCollectGarbageImpl<1>(EObjectFlags) GarbageCollection.cpp:5580 [UnrealEditor-CoreUObject.dll] UE::GC::FReachabilityAnalysisState::PerformReachabilityAnalysisAndConditionallyPurgeGarbage(bool) GarbageCollection.cpp:5770 [Inlined] [UnrealEditor-CoreUObject.dll] UE::GC::CollectGarbageInternal(EObjectFlags, bool) GarbageCollection.cpp:5342 [UnrealEditor-CoreUObject.dll] CollectGarbage(EObjectFlags, bool) GarbageCollection.cpp:6007 [UnrealEditor-UnrealEd.dll] UResavePackagesCommandlet::LoadAndSaveOnePackage(const FString &) ContentCommandlets.cpp:1014 [UnrealEditor-UnrealEd.dll] UResavePackagesCommandlet::Main(const FString &) ContentCommandlets.cpp:1374 [UnrealEditor.exe] FEngineLoop::PreInitPostStartupScreen(const wchar_t *) LaunchEngineLoop.cpp:4231 [Inlined] [UnrealEditor.exe] FEngineLoop::PreInit(const wchar_t *) LaunchEngineLoop.cpp:4532 [Inlined] [UnrealEditor.exe] EnginePreInit(const wchar_t *) Launch.cpp:46 [UnrealEditor.exe] GuardedMain(const wchar_t *) Launch.cpp:185 [UnrealEditor.exe] LaunchWindowsStartup(HINSTANCE__ *, HINSTANCE__ *, char *, int, const wchar_t *) LaunchWindows.cpp:247 [UnrealEditor.exe] WinMain(HINSTANCE__ *, HINSTANCE__ *, char *, int) LaunchWindows.cpp:298And that’s running the lightmapper with stomp malloc.
This is a command that comes from the farm and it looks like this:
-run=ResavePackages -buildtexturestreaming -buildlighting -MapsOnly -ProjectOnly -AllowCommandletRendering -SkipSkinVerify -Map=X -fileopenlog -AutoCheckOutPackages -LightingScenario=X -stdout -unattended -NoLogTimes -stompmallocIt happens after all of the lighting baking is done, just before the Editor finishes, but looking at the stack, it seems to be actually as the GC collects things.
I cannot tell which FInstancedStruct it is without further changes since by this point the referenced memory is already garbage, but since it’s being destroyed as part of the GC, it seems that it was already part of a UPROPERTY somehow?
Is this useful info?
I’m guessing it’s that one?
https://github.com/EpicGames/UnrealEngine/commit/c16e4d7e4a9892470df6265da6fdaa59f6f03668
It seems a bit big, I will try and do check it once we upgrade to the next version of the Engine.
Thank you!