Memory leaks if GDALAllRegister is not called

[Mateusz Łoskot]

Let's consider this code as a complete program calling GDAL (DLL) on Windows:

int main()
{
  GDALDestroy(); // or, DllMain at DLL_PROCESS_DETACH
}

It causes GDAL to leak memory and OS kernel resource in several places. Here is corrected version which clears all memory, comments point out what is actually cleared:

int main()
{
  GDALDestroy();   // calls CPLDebug > (...)
    
  CPLFreeConfig(); // cleans after: CPLDebug > CPLGetConfigOption > mutex
  CPLCleanupTLS(); // cleans after: CPLDebug > CPLGetErrorContext > TlsAlloc + VSICalloc
  CPLCleanupMasterMutex(); // cleans after: CPLDebug > CPLGetConfigOption > mutex
}

BTW, CPLDebug was added to GDALDestroy in r21261.

Actually, it does not have to be GDALDestroy. Client may call CPLSetConfigOption or similar non-driver/dataset related routine. It will allocate resources which are neither cleared by GDALDestroy nor GDALDestroyDriverManager, obviously.

The trick that is required to trigger complete clean up is to register drivers, so driver manager is fully created. Then, this program does not leak anything:

int main()
{
  GDALAllRegister();
  GDALDestroy(); // or, DllMain at DLL_PROCESS_DETACH
}

The leaks mentioned above are reported by Intel Inspector XE as well as ​Visual C++ CRT memory debugger. Allocation by CPLGetErrorContext > VSICalloc is reported as ​Memory leak, while others as ​Memory not deallocated (still reachable in Valgrind speak).

This suggests there are several issues:

• GDALDestroy should be intuitively a no-op if no prior calls to any of GDAL routines has been made.
• GDALDestroy has implicit requirement of prior call to GDALAllRegister (not always a case, especially in non-trivial scenarios).
• The clean up tree rooted at GDALDestroy has in fact circular dependency at CPLDebug.
• Memory not deallocated (Valgrind: still reachable) reports might be just warnings, but they cause unnecessary noise while debugging memory issues.

[Mateusz Łoskot]

Patch to trigger clean-up steps missing in case GDALAllRegister is not called.

[Kurt Schwehr]

Took a quick pass through gdaldrivermanager.cpp to see what is happening in there and did a bit of cleanup with r30798. I can't say I understand the issues you are seeing yet.

[Mateusz Łoskot]

Replying to goatbar:

Took a quick pass through gdaldrivermanager.cpp to see what is happening in there and did a bit of cleanup with r30798.

AFAICT, those changes do not really address the root of the issues I'm describing; leaks happen without any driver is registered. I believe I gave clear examples of all circumstances and what happens when.

If my patch is kosher for everyone, I'd suggest to apply it. It fixes all the issues I'm reporting.

[Even Rouault]

Shouldn't the 3 new functions calls be done in GDALDestroy() itself (instead in Windows specific code path) ?

[Mateusz Łoskot]

Replying to rouault:

Shouldn't the 3 new functions calls be done in GDALDestroy() itself

Possibly, but I couldn't test it on non-Windows setup and recalling some previous issues with TLS related crashes on Unix, I decided to go for the Windows-only fix.

[Even Rouault]

Trying the multiplatform way. Seems to work well on Linux too.

trunk r30813 "Fix memory leaks if GDALAllRegister is not called (derived from patch by mloskot, #6139)"

[Kurt Schwehr]

mloskot, I wasn't trying to fix the issue. Just that this issue motivated me to read that code and I might as well try to make that code better if I'm putting time into it.

[Even Rouault]

All bots seem to be happy. Closing

[Mateusz Łoskot]

I haven't checked, but I'm confident the modified patch fixes the issues.

Thank you Even.

[Mateusz Łoskot]

Even, can we have the patch merged into branches/2.0 too?

[Even Rouault]

Probably. I let you do the backport ?

[Mateusz Łoskot]

branches/2.0 r30852:

Merged r30813 from trunk: Fix memory leaks if GDALAllRegister is not called (derived from patch by mloskot, #6139)

I have confirmed Even's modified patch fixes the problem on Windows.

[Mateusz Łoskot]

Linking related ticket #6868 just submitted.