locale0.cpp: Protect memory integrity of _Fac_node

[li-boxuan]

locale0.cpp has a global static linked list:

STL/stl/src/locale0.cpp

Line 52 in 8af2cc4

__PURE_APPDOMAIN_GLOBAL static _Fac_node* _Fac_head = nullptr;

whose memory is released when another global variable,

STL/stl/src/locale0.cpp

Line 64 in 8af2cc4

__PURE_APPDOMAIN_GLOBAL const _Fac_tidy_reg_t _Fac_tidy_reg;

, is destructed:

STL/stl/src/locale0.cpp

Lines 54 to 62 in 8af2cc4

	struct _Fac_tidy_reg_t {
	~_Fac_tidy_reg_t() noexcept { // destroy lazy facets
	while (_Fac_head != nullptr) { // destroy a lazy facet node
	_Fac_node* nodeptr = _Fac_head;
	_Fac_head = nodeptr->_Next;
	delete nodeptr;
	}
	}
	};

This is fine, except when new and delete are replaced globally by application code. In our application code (a fairly old codebase that is hard to refactor), we replace new and delete operators globally and use our private custom heap to manage the memory. The problem is _Fac_node linked list's memory is allocated on our private heap, but when the destructor of _Fac_tidy_reg_t is invoked, our private heap has already been destroyed, causing read access violation. This crash doesn't happen in DEBUG build, where _Fac_node has its own implementation of new and delete implementation.

This PR proposes that _Fac_node overrides new and delete in non-DEBUG mode as well, not only for consistency with DEBUG mode, but also for integrity and control of its memory usage.

[frederick-vs-ja]

The problem is _Fac_node linked list's memory is allocated on our private heap, but when the destructor of _Fac_tidy_reg_t is invoked, our private heap has already been destroyed, causing read access violation.

I think your private heap is incorrectly implemented and hence doesn't meet the required behavior in the Standard. So such use causes undefined behavior, and thus MSVC STL doesn't need to defend against it.

[CaseyCarter]

I think your private heap is incorrectly implemented and hence doesn't meet the required behavior in the Standard. So such use causes undefined behavior, and thus MSVC STL doesn't need to defend against it.

While I agree with this as an argument against the presented reason for the change, I'm willing to consider that we should be using the same form of allocation in both debug and release builds. Variations between debug and release can hide problems. This is why I've tagged the PR as "enhancement" and not "bug".

I haven't approved yet since I'm not completely convinced, but I think it at least merits discussion.

[li-boxuan]

This is why I've tagged the PR as "enhancement" and not "bug".

I agree it's an enhancement, not a bug, and I do recognize the fundamental design issue of our application code. That being said, I wouldn't say our implementation is "incorrect" - it seems inevitable when you decide to globally replace new and delete operators? Application code has no control of when _Fac_tidy_reg_t is destroyed, and to my best knowledge, there's no way for us to enforce our private heap is destroyed after _Fac_tidy_reg_t is destroyed. This prevents us from shutting down gracefully.

[li-boxuan]

Unpopular opinion: I argue that the current design of _Fac_node and _Fac_tidy_reg_t is not flawless. It is initialized before main, could grow (new pointers added to the linked list) any time throughout the lifecycle of the application, and destroyed after main. This seems to prevent any non-hacky effort from replacing new and delete operators globally, which IMHO, is a valid practice (although it might not be the best and most popular practice).

[StephanTLavavej]

This is the absolute scariest part of the STL, and it's a miracle that it works as-is, much less when people are replacing operator new and operator delete.

The proposed change looks reasonable to me but I also need to think about it (and get past my sheer blinding terror of any changes to the facet machinery; I've been dealing with breakage in this area since 2007).

[StephanTLavavej]

I'll approve this. There's Only One STL^TM so there are no mix-and-match concerns, and avoiding release/debug differences is a benefit. I'm not especially persuaded by the application scenario (our behavior has been like this for a long time - why is it a problem now?), but having only one form of STL behavior is strictly simpler.

_Crt_new_delete still conditions its behavior on _DEBUG, but because it's both separately compiled (virtually certain to appear on the DLL's export surface; haven't verified) and header-visible, we really shouldn't mess with it in the binary-compatible era.

STL/stl/inc/xlocale

Lines 31 to 33 in 8af2cc4

	extern "C++" struct _CRTIMP2_PURE_IMPORT _Crt_new_delete { // base class for marking allocations as CRT blocks
	#ifdef _DEBUG
	void* __CLRCALL_OR_CDECL operator new(size_t _Size) { // replace operator new

[li-boxuan]

Thanks for the discussion and approval. Great to see agreement on merging this PR, despite for a different reason than my original motivation.

I'm not especially persuaded by the application scenario (our behavior has been like this for a long time - why is it a problem now?)

Why it used to work is a mystery to me. By setting up a debugger, I clearly see _Facet_Register being called after our private heap is initialized, and ~_Fac_tidy_reg_t being called after our private heap is destroyed, which means _Fac_tidy_reg_t destructor would attempt to traverse and delete a linked list that isn't valid anymore.

FWIW our C++ project is integrated into a C# project and we have managed C++ code that makes things more complicated. We are migrating from .Net framework to .Net core which might somehow change something magic.

[CaseyCarter]

I haven't approved yet since I'm not completely convinced, but I think it at least merits discussion.

I haven't come up with a reason not to accept this change. It improves debug/release consistency a bit and causes no harm.

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[StephanTLavavej]

Thanks for simplifying this ancient horror, and congratulations on your first microsoft/STL commit! 😻 🎉 🚀

Because I like to live dangerously, this change is expected to ship in VS 2022 17.12 Preview 3.