Memory leak in Singleton::Ref()?

[kravlost]

Since moving to VS2017 with Crypto++ compiled through vcpkg I've started to see a memory leak. Crypto++ is statically linked into a DLL which is loaded by the main program. One of the leaks has this call stack when I load the DLL when running a unit test:

ntdll.dll!RtlAllocateHeap 771113b0
ucrtbase.dll!malloc_base 76978d96
MyDll.dll!CryptoPP::UnalignedAllocate Line 256 + 0x14 bytes 0f6078a4
MyDll.dll!CryptoPP::Integer::Integer Line 2926 + 0x55 bytes 0f60c995
MyDll.dll!CryptoPP::Singleton<CryptoPP::Integer,CryptoPP::NewInteger<2>,0>::Ref Line 303 + 0x9f bytes 0f60f4df
MyDll.dll!CryptoPP::`anonymous namespace'::`dynamic initializer for 's_two'' Line 3047 + 0xc bytes 0f6010bc
ucrtbase.dll!initterm 769729e3
MyDll.dll!dllmain_crt_process_attach Line 63 + 0x8c bytes 0f61d258
MyDll.dll!dllmain_crt_dispatch Line 137 + 0x3b bytes 0f61d1b4
MyDll.dll!dllmain_dispatch Line 194 + 0x6e bytes 0f61d3c1
MyDll.dll!_DllMainCRTStartup Line 252 + 0x1c bytes 0f61d4a2
ntdll.dll!WinSqmAddToStream 7713e726
ntdll.dll!RtlDeactivateActivationContextUnsafeFast 7710cbef
ntdll.dll!LdrShutdownProcess 77107baa
ntdll.dll!LdrShutdownProcess 77107a44
ntdll.dll!LdrShutdownProcess 77107a63
ntdll.dll!RtlIsCriticalSectionLockedByThread 7712014d
ntdll.dll!RtlPrefixUnicodeString 77109012
ntdll.dll!LdrControlFlowGuardEnforced 7711b63e
ntdll.dll!LdrLoadDll 7711e8de

Could this be related to #372 or is it a problem with the way vcpkg compiles the static library?

Can I safely ignore this?

Edit: I'm compiling with the /permissive- flag and C++14

---

Windows 10 64-bit VS2017 Pro 15.5.2 Crypto++ 5.6.5 vcpkg version

[noloader]

Could this be related to #372 or is it a problem with the way vcpkg compiles the static library?

Yeah, this is a known problem with Microsoft platforms. Microsoft claims to support C++11 but they don't implement Dynamic Initialization and Destruction with Concurrency. I think it tool them 12 years or so to implement the core C++11 feature. When Microsoft implemented it, they required Windows 10 and above with programs built using VS 2017 and above. (In contrast, GCC and LLVM provided the feature when it was still a draft, long before 2008).

It really boils down to a "pick your poison" on Microsoft platforms. We had to error on the side of caution. Caution led us to memory leaks over loader hangs and runtime crashes.

If you want to avoid the memory leaks, then you can compile with /D CRYPTOPP_CXX11_DYNAMIC_INIT=1. CRYPTOPP_CXX11_DYNAMIC_INIT=1 is set automatically when the platform supports it. But be aware you could be subject to hangs like Issue 373, Loader lock on WinXP.

We also found good success with /D USE_MSC_INIT_PRIORITY=1, but it is optional because it is an OS-specific feature, and not a C++ feature. I personally like /D USE_MSC_INIT_PRIORITY=1. It does not depend on Microsoft's (lack of) Dynamic Initialization and Destruction with Concurrency. It works with C++03, C++11, C++14 and beyond; and it works with Windows XP, Vista, Win7, Win8 (and we believe beyond).

I believe most of the leaks are due to Integer class. You might also be interested in the head notes at cryptopp/integer.cpp. You can also see how the code reacts to /D USE_MSC_INIT_PRIORITY=1.

---

Can I safely ignore this?

Yeah, that's about all we can do unless you satisfy >= Win10 and >= VS2017.

[ras0219-msft]

@noloader Would you recommend that we (vcpkg) always build with USE_MSC_INIT_PRIORITY?

[noloader]

Would you recommend that we (vcpkg) always build with USE_MSC_INIT_PRIORITY?

Yes, I do. That's assuming you are supporting Windows XP or Vista and above.

You can add USE_MSC_INIT_PRIORITY=1 as a command line option for just integer.cpp because that's the only place it is used at the moment. Or you can uncomment the define in the integer.cpp source file.

Prior to some of reworking of the Singletons we were seeing 70 to 130 memory leaks due to the Singleton. I think we have it reduced to a handful of them now.

If you are a distro, packager or maintainer (doing this stuff for the benefit of others), then you can forgo these trackers/reporters and ping me directly. noloader, gmail account.

[kravlost]

Thanks for all the feedback and insights - very interesting.

I will ignore the errors for now and update Crypto++ in vcpkg when that is available.

[noloader]

@steronydh,

No problems with feedback and untangling this. It was a painful learning curve for me. When we saw Microsoft claimed C++11 support we moved to use C++11 primitives. But Microsoft did not really support C++11, so we started catching bugs like Issue #372 and Issue #373.

After all the gyrations, Wei's original Singleton pattern was/is the best generalized solution, assuming you want to avoid loader locks and crashes at the expense of some memory leaks. It worked in the 1990's and it still works today.

We found we were able to improve the memory leaks by using Microsoft init_seg and GCC's init_priority. But they are only available on Microsoft and Linux platforms; and they move away from C++ and toward platform specific remediations. And we still lack something platform specific for AIX, iOS, OS X, Solaris and several other OSes.

---

In the bigger picture, this is due to Static Initialization Order Fiasco. We had about five of them that could cause problems. We got rid of the statics for DEFAULT_CHANNEL and AAD_CHANNEL. But we still have three of them for Integer::Zero(), Integer::One(), and Integer::Two().

Another related problem is thread local storage (TLS), and each thread getting its own copy of values like Integer::Zero(), Integer::One(), and Integer::Two(). TLS is the reason for the XP loader hang. There are some hidden locks used behind the scenes when using static variables. When modules are loaded under Windows, the loader uses a lock too. When a DLL is loaded - Bam - deadlock. The lockless Singleton that leaks memory is the way to sidestep it. Microsoft finally rewrote things to avoid the deadlock, but you need >= Win10 and >= VS2017.

[kravlost]

This is an implementation detail and I guess OT, but why is there a separate Integer::Zero(), One() and Two() for just those values?

[noloader]

why is there a separate Integer::Zero(), One() and Two() for just those values?

Each Integer is backed by a heap allocation. The allocation is small, and it consists of two double words (8 bytes). 0, 1 and 2 are used frequently enough to create them once, make them const, and reuse them. Reusing them also helps to avoid heap fragmentation with lots of little allocations.

[noloader]

@steronydh,

Is there anything else that needs attention here? If not, can we close it?

[kravlost]

@noloader I don't think so, thanks, so yes, please close.

[noloader]

@steronydh, Ack, thanks. Ping me if you need it. noloader, gmail account.

[cstamatopoulos]

There is a single memory leak in my case and uncommenting the USE_MSC_INIT_PRIORITY define does not help. However, in my case it seems that the Integer stuff is not involved but instead it is CryptoPP::Singleton<CryptoPP::PKCS1v15_SignatureMessageEncodingMethod,CryptoPP::NewObject<CryptoPP::PKCS1v15_SignatureMessageEncodingMethod>,0>::Ref()

Win 10, VS 2017 v.15.6.7 which is the latest.