<chrono>: Possible way forward to avoid `tzdb`'s memory leak

[mnatsuhara]

[ Copied over from DevCom-1644641 ]

[severity:It’s more difficult to complete my work]

This bug has already been reported at https://developercommunity2.visualstudio.com/t/std::chrono::current_zone-produces-a/1513362 before, but that issue got closed.

Repro Program: chronoleak.cpp:

#define _CRTDBG_MAP_ALLOC
#include <stdlib.h>
#include <crtdbg.h>

#include <chrono>

int main() {
        _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE);
        _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDERR);
        _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE);
        _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDERR);
        _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE);
        _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);
        _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
        {
                std::chrono::get_tzdb_list();
        }
        return 0;
}

Compile command: cl /std:c++20 /permissive- /EHsc /MTd chronoleak.cpp

Output see attached leaks.txt (memory leaks reported)

I disagree with the comment in https://developercommunity2.visualstudio.com/t/std::chrono::current_zone-produces-a/1513362#T-N1516296.

I think wrapping inline atomic<tzdb_list*> _Global_tzdb_list; (see https://github.com/microsoft/STL/blob/59a87ccc947ed4ccefd3b87e51b6a7136341c674/stl/inc/chrono#L2852>) in a struct that has a destructor could run the required cleanup. If destruction order on process shutdown is a concern, I think the standard allows throwing if it would be called after the containing object had already been destroyed. See C++20 [time.zone.db.access]:

Throws: runtime_error if for any reason a reference cannot be returned to a valid tzdb_list containing one or more valid tzdbs.

This memory leak blocks adoption of C++20 std::chrono functionality in OpenMPT and libopenmpt (https://github.com/OpenMPT/openmpt/) because the resulting noise makes using the CRT memory leak detector impractical for us. In our case, tzdb is loaded via a call to std::chrono::from_stream() as in:

std::chrono::utc_clock::time_point tp{};
std::istringstream s;
s.imbue(std::locale::classic());
s.str("2022-01-01 23:42");
std::chrono::from_stream(s, "%F %R", tp);

Related Context

• 2047

• DevCom-1513362

Further ABI Considerations

As noted in #2047, we were initially skeptical that we could address the memory leaking issue in (1) a Standard-compliant way and (2) without breaking ABI as <chrono> is ABI-locked under /std:c++20. However, given the suggestion by the author of this DevCom issue along with the cited permission to fail ([time.zone.db.access]), we believe we could fix the memory leaking while remaining Standard-conformant. Further, @StephanTLavavej suggests that perhaps renaming the _Global_tzdb_list variable to something like _Global_tzdb_list_v2 would allow us to make this change without breaking ABI.

Filing this issue to track the need to investigate this potential fix for this issue.

Tracked by VSO-1471620 / AB#1471620 .

[AlexGuteniev]

Can you also unleak dll handle #2316 ?

[schlzber]

Maybe this helps, as for the same reason I added this "cleanup code" during program shutdown (in desctructor of CWinApp derived class in MFC project):

#ifdef _DEBUG
    // to prevent the tzdb allocations from being reported as memory leaks
    std::chrono::get_tzdb_list().~tzdb_list();
#endif

[StephanTLavavej]

I haven't checked, but it might be possible to allocate CRT blocks in these vectors, because they can still be freed via the normal mechanism.