Proper cleanup after calling Interpreter::addExternFunction and MakeExternObjectFunction

[ataylor]

I am tracking down memory leaks in our application with the aid of the CRT library in Windows.

 

Many, many leaks appear to originate from various calls to Unigine::MakeExternObjectFunction. Example call from the constructor for our derived Unigine::App --

Unigine::Interpreter::addExternFunction("MyApp.DoThing", Unigine::MakeExternObjectFunction(this, &MyApp::DoThing));

We are also calling Interpreter::removeExternFunction in MyApp destructor:

Unigine::Interpreter::removeExternFunction("MyApp.DoThing");

But it seems that the ExternFunctionBase returned by MakeExternObjectFunction still lives on.

 

I discovered that I can save the pointer returned by MakeExternObjectFunction and delete it later in MyApp destructor, which fixes the memory leak. Is this proper procedure? I assumed calling removeExternFunction would take care of this or that the engine would otherwise somehow automatically manage its lifetime, but perhaps not.

 

We are currently using Unigine 2.2.1.

[ataylor]

It seems this extends to all available Unigine::MakeExtern________ calls.

[shodan]

I might need to debug this a bit more, but at a quick glance I can not fully confirm this. To clarify, why specifically do you suspect that the object still lives? Also, do you debug with USE_MEMORY enabled, or did you disable it?

 

My findings so far are that the external API method, ie. void Unigine::Interpreter::removeExternFunction(const char *name), seems to be cleaning up the actual internal engine object alright. (Rather vigorously, too.)

 

That said, the lightweight wrapper object returned from the very first MakeExternObjectFunction() might still exist indeed. I guess that bit was essentially overlooked when exposing the internal API. However, after removeExternFunction() that wrapper object should not be pointing to any real internal objects anymore. That's still a leak, of course, but at least it's a tiny one.

 

I am going to double check that and then change the behavior of addExternFunction() to destroy any such temporary wrappers, at least optionally. Thanks for the report!

[ataylor]

I suspect the object still lives for multiple reasons:

 

As I said, I am using the CRT library to help with this and following the procedure outlined here (https://msdn.microsoft.com/en-us/library/x98tx3cf(v=vs.120).aspx). Many of the memory allocation numbers reported as leaks on app shutdown are those of calls to MakeExternObjectFunction on app startup.

 

When I save the pointers returned by MakeExternObjectFunction and delete them manually on app shutdown, those allocations are no longer reported as leaks by the CRT library.

 

If these ExternFunctionBase objects were indeed already being deleted, I would expect the manual deletions I added to cause a double-delete crash. But this is not happening.

 

A breakpoint placed in the dtor for ExternFunctionBase does not hit without my delete calls in place.

 

And lastly, if I step into the removeExternFunction call, it seems the conditions are never satisfied to enter into this while block:

while((extern_function = ::EngineInterpreter::removeExternFunction(name)) != NULL) {

USE_MEMORY is enabled.

[ataylor]

Sorry, I'm still a little confused about this. Is it expected to have to save and delete the pointer returned from MakeExternObjectFunction? Or is it expected that removeExternFunction will delete it?

[silent]

Hi Adam,

 

We are still investigating. As soon as we have more details we will sure let you know. Right now it looks like a small memory leak and potentially a bug.

 

Thanks!

[shodan]

Hi there,

 

apologies for the delayed followup, been a bit busy, but just managed to take another pass at this.

 

And lastly, if I step into the removeExternFunction call, it seems the conditions are never satisfied to enter into this while block:

while((extern_function = ::EngineInterpreter::removeExternFunction(name)) != NULL) {

 

This is the root cause. And this is probably caused by an "unexpected" shutdown preceding the remove...() calls.

 

1. I confirm that you are not really supposed to store pointers returned by MakeObjectExternFunction. The code within that quote while() loop cleans up both the internal engine object and the wrapper object. Specifically the ExternFunctionBase::release_ptr(ret); line kills the wrapper.

 

2. I confirm that normally, everything works as expected, ie. the wrapper gets destroyed. I made a small test (changed Lights.cpp sample actually) that does addExternFunction() in MyApp constructor, then does removeExternFunction() in destructor, and I also changed static void release_ptr(ExternFunctionBase *ptr) in include/UnigineInterface.h to print out the pointer being deleted. And the wrapper pointer returned from addExternFunction() got deleted just fine, as expected.

 

3. If the contents of the while() loop are never executed for you, then it's likely that by the time you attempt removeExternFunction() the interpreter bindings are already destroyed somehow.

 

4. But the respective destroy() functions are not exposed via the API and can't be easily called. My "best" malicious attempt so far was to explicitly call Engine::shutdown() from within MyApp destructor. Just before the removeExternFunction() call. That worked, for the purposes of repeating the behavior you describe. But, obviously, that also immediately caused not just that minor leak but a bunch of other crazy crashes :)

 

All that said, I suppose you init and shutdown the engine multiple times in your app? In that case, you currently need to remove the bindings before shutting down, and add them again after initializing the engine again. I am going to modify the interpreter destruction to avoid the former, ie. to auto-remove the bindings. However, the latter, ie. the re-adding of userland extern functions, will need to be added into your app anyway.

 

And if you don't... then please add a breakpoint to Interpreter::destroy() and see why does it even get called.