Shutdown crash under mallocstomp2 when a module uses StaticClass to unregister a delegate

Development Pipeline & Plugins
, ,
1

I have been testing editor runs with FMallocStomp2 for a couple days looking for some memory trampling issues. Yesterday I hit a shutdown crash that looks like it throws into question a commonly used pattern in module code. With mallocstomp allocators, memory gets trampled as soon as it is freed, so use-after-free bugs get thrown into high contrast.

The crash was in this “experimental” code. The specific bit was GetFName() being invoked on a bad pointer. The static class had already been destroyed by the engine shutdown. Surveying other uses of UnregisterCustomClassLayout() shows about half of the cases cache the FName from earlier and re-use it, but that leaves about half doing this StaticClass()->GetFName() call pattern.

If that’s an unsafe pattern, it should be scrubbed from the codebase. If it’s intended to be a safe pattern and isn’t, you should take a look at the order of operations between the static class registration and module manager shutdown.

[Content removed]

void FHairCardGeneratorEditorModule::ShutdownModule()
{
  HairCardGenerator_Utils::UnregisterModularHairCardGenerator(this);

  // Register custom detail panel handler for hair card settings
  FPropertyEditorModule& PropertyModule = FModuleManager::LoadModuleChecked<FPropertyEditorModule>("PropertyEditor");
  PropertyModule.UnregisterCustomClassLayout(UHairCardGeneratorPluginSettings::StaticClass()->GetFName());
}
2

Steps to Reproduce
I doubt this is a 100% repro kind of bug, but I’ll try again today and see if I can get it to happen again. But because someone else hit it on here, it’s clearly something that can happen

  1. Run the editor with mallocstomp2. Eg: `GamName -stomp2malloc -MallocStomp2MaxSize=262143`
  2. Play PIE a couple times
  3. Close the editor
3

I was able to reproduce the problem. And now I’ve spent some additional time figuring out when exactly CDO objects get destroyed relative to the module manager shutdown to prove there’s a concrete problem.

CDOs get destroyed at FEngineLoop::AppPreExit() in the following delegate broadcast:

FCoreDelegates::OnExit.Broadcast();

FModuleManager::Get().UnloadModulesAtShutdown() gets called during FEngineLoop::Exit(), which is well after PreExit. Any use of CDOs at this point is invalid.

4

Hi ,

The problem is that StaticExit is called before the modules are shutdown. StaticExit destroys all the still existing UObjects so the UHairCardGeneratorPluginSettings::StaticClass is invalid at that point but still accessible since static…

This has likely implication for other ShutdownModule methods in the engine. I am discussing the problem with the development team to figure out what is the best resolution here.

Martin

5

I show 57 uses of that StaticClass() call in module shutdown in my codebase

6

I decided to try out adding a test for whether the UObjects are alive in the macro thing that generates StaticClass() functions. It seems there’s some use of it before UObjectInitialized() goes true, but this test generally seemed like it was scoped correctly to identify problems:

checkSlow(!IsEngineExitRequested() || UObjectInitialized()); \What I spotted was a large-ish number of uses of UObjectInitialized() checks are already in the various module shutdown calls. People have been fighting this issue sporadically, I’d suppose. With the checkSlow() in there, I have our engine cleaned up enough that MallocStomp2 no longer complains. I needed to edit 20 files, the majority of which are Epic controlled, but a good handful of uses in our code where people have copied an invalid pattern.

By and large, those edits look like wrapping some or all of the content of ShutdownModule() functions with if (UObjectInitialized()) to conditionally skip the unsafe reads.

What do you think? Is that the right approach? Alternatively, one could use a different pattern for getting the FNames, but I’ll note a couple uses of StaticClass() that weren’t just for getting the UObject FName.

7

Hi Alexander,

Yes, this is an unfortunate problems in UE where a lot of systems owners might not have properly understood the proper module shutdown flow. Fixing those bad usage pattern can be done with the approach you’ve just mentioned (to check if the UObject system is initialized). This is certainly a way to avoid the issue which will end up being correct here. The other pattern you might see in a few areas, (which I would consider a bit more correct) is module hooking up to `OnPreExit` on startup, put all their UObject related work in the hooked callback which they also call on shutdown in case the module is loaded/unloaded during the span of the application. if you already have the filenames on end of the UE code you had to fix then I’d appreciate you sharing it so that we can address it on our side as well.

Thank you for raising this issue up. It’s quite appreciated!

Cheers!

Francis

8

The files in question are:

/Engine/Plugins/Animation/LiveLinkHub/Source/LiveLinkHub/Private/LiveLinkHubModule.cpp

/Engine/Plugins/Developer/Concert/ConcertApp/MultiUserClient/Source/MultiUserClient/Private/MultiUserClientModule.cpp

/Engine/Plugins/Editor/GeometryMode/Source/BspMode/Private/BspModeModule.cpp

/Engine/Plugins/Experimental/ActorModifierCore/Source/ActorModifierCoreEditor/Private/ActorModifierCoreEditorModule.cpp

/Engine/Plugins/Experimental/CineCameraRigs/Source/CineCameraRigsEditor/Private/CineCameraRigsEditor.cpp

/Engine/Plugins/Experimental/HairCardGenerator/Source/HairCardGeneratorEditor/Private/HairCardGeneratorEditorModule.cpp

/Engine/Plugins/Experimental/PropertyAnimatorCore/Source/PropertyAnimatorCoreEditor/Private/PropertyAnimatorCoreEditorModule.cpp

/Engine/Plugins/Media/AjaMedia/Source/AjaMediaEditor/Private/AjaMediaEditorModule.cpp

/Engine/Plugins/MovieScene/LevelSequenceEditor/Source/LevelSequenceEditor/Private/LevelSequenceEditorModule.cpp

/Engine/Plugins/MovieScene/TemplateSequence/Source/TemplateSequence/Private/TemplateSequenceModule.cpp

/Engine/Plugins/MovieScene/TemplateSequence/Source/TemplateSequenceEditor/Private/TemplateSequenceEditorModule.cpp

/Engine/Plugins/Mutable/Source/CustomizableObjectEditor/Private/MuCOE/CustomizableObjectEditorModule.cpp

/Engine/Plugins/PCG/Source/PCG/Private/PCGModule.cpp

/Engine/Plugins/VirtualProduction/VirtualCameraCore/Source/VCamCoreEditor/Private/VCamCoreEditorModule.cpp

/Engine/Source/Editor/PixelInspector/Private/PixelInspectorModule.cpp

/Engine/Source/Runtime/Engine/Private/Streaming/RenderAssetUpdate.cpp

9

Thank you! Will follow up internally on those.

Francis

10

There’s a new set of problems under 5.6.1, which we recently integrated. I haven’t fully worked through the problem set, but using `-stompmalloc` (because FMallocStomp2 no longer works in the editor) is highlighting a bunch of new shutdown crashes that all get hung up in this little blob of `UCaptureManagerEditorSettings::Initialize()`. Module after module is using a shutdown hook that loads this module and crashes here:

```

IModularFeatures::Get().OnModularFeatureUnregistered().AddLambda(

\[this](const FName\& InFeatureName, IModularFeature\* InFeature)

{

  if (InFeature \=\= this\-\>LiveLinkClient)

  {

    this\-\>LiveLinkClient \= nullptr;

  }

}

);

I’ve hit 4 of these so far. An example:

```

void FRewindDebuggerVLogModule::ShutdownModule()

{

#if ENABLE_REWINDDEBUGGER_VLOG_INTEGRATION

IModularFeatures::Get().UnregisterModularFeature(IRewindDebuggerExtension::ModularFeatureName, &RewindDebuggerVLogExtension);

IModularFeatures::Get().UnregisterModularFeature(TraceServices::ModuleFeatureName, &VLogTraceModule);

IModularFeatures::Get().UnregisterModularFeature(RewindDebugger::IRewindDebuggerTrackCreator::ModularFeatureName, &GVisualLogTrackCreator);

#endif

}

```

The debugger is telling me the `TBaseDelegateFunctionInstance.Functor.__this` field is bad memory. I haven’t figured out the mechanics of this or a workaround yet.

```

11

Hi Alexander, thanks for reporting these additional issues, I will follow up on those new instances with you and get back to you

12

Adding a few additional crashing modules here, testing with mallocstomp and 5.6.1

SourceControlModule.cpp -- GetProvider().Close() appears to be using deleted memory

MetaHumanSpeech2FaceModule.cpp -- same StaticStruct() business as the original report

LiveLinkHubModule.cpp -- StaticClass()

ModularFeatures.cpp:

```

void FModularFeatures::UnregisterModularFeature( const FName Type, IModularFeature* ModularFeature )

{

UE::TScopeLock ScopeLock(ModularFeaturesMapCriticalSection);

ModularFeaturesMap.RemoveSingle( Type, ModularFeature );

// This Broadcast appears to be using deleted memory

if (!IsEngineExitRequested())

{

ModularFeatureUnregisteredEvent.Broadcast(Type, ModularFeature);

}

}

```

13

Hi,

I wanted to provide some feedback. The problems have been reported to the different parts of the engine team. I unfortunately don’t have ETAs to share at this point.

Regards,

Martin