<codecvt>: wstring_convert leaks memory

[BillyONeal]

Describe the bug The standard says that wstring_convert is to delete the supplied facet, but we don't appear to be doing that. See https://eel.is/c++draft/depr.conversions.string#18

The current implementation puts the facet into a std::locale object which may or may not delete the facet on completion.

Command-line test case STL version (git commit or Visual Studio version):

C:\Users\bion\Desktop>type test.cpp
#define _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING
#include <assert.h>
#include <locale>
#include <stdio.h>

int fancyAlive = 0;

class fancy_state {};
class fancy_cvt : public std::codecvt<wchar_t, char, fancy_state> {
public:
  explicit fancy_cvt(size_t refs = 0)
      : std::codecvt<wchar_t, char, fancy_state>(refs) {
    ++fancyAlive;
  }
  ~fancy_cvt() {
    --fancyAlive;
    puts("destroyed!");
  }

  fancy_cvt(const fancy_cvt &) = delete;
  fancy_cvt &operator=(const fancy_cvt &) = delete;
};

int main() {
  {
    fancy_cvt *p = new fancy_cvt(5);
    assert(fancyAlive == 1);
    std::wstring_convert<fancy_cvt> cvt(p);
  }
  assert(fancyAlive == 0);
  puts("main() ending");
}

C:\Users\bion\Desktop>cl /EHsc /W4 /WX /std:c++latest /nologo .\test.cpp
test.cpp

C:\Users\bion\Desktop>.\test.exe
Assertion failed: fancyAlive == 0, file .\test.cpp, line 30

C:\Users\bion\Desktop>

This is a dual of Microsoft-internal VSO-364730 / AB#364730.

vNext note: Resolving this issue will require breaking binary compatibility. We won't be able to accept pull requests for this issue until the vNext branch is available. See #169 for more information.

[BillyONeal]

This is a dual of Microsoft-internal VSO-365507 and its duplicate VSO-364730.

[rlintott]

Is this doable for a first time contributor?

[BillyONeal]

@rlintott Unfortunately it's ABI breaking to fix, that's why it's tagged vNext.

[StephanTLavavej]

(Which means that we're unable to fix this in the VS 2019 release series, regardless of contributor. We'll be starting work on a "vNext" binary-incompatible release at some point in the future, but the timing isn't certain and is many months away for sure.)

[rfog]

Is this solved in VS 2022? If this is marked as obsolete in C++17, why there is no replacement for C++20?

[BillyONeal]

Is this solved in VS 2022?

VS2022 is still ABI compatible with previous releases, so no.

If this is marked as obsolete in C++17, why there is no replacement for C++20?

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0618r0.html

There's no reasonable way to report errors in a way consistent with Unicode, nor was it implemented correctly by any of the implementations (although this one is particularly bad).

Notably, the main reason you would want something like this would be so that it would work with std::fstream but it doesn't even work for that, since no UTF-8 <-> UTF-16 conversion can comply with the 1:N constraint ( http://eel.is/c++draft/locale.codecvt#virtuals-4 ) because 3 UTF-8 encoding units result in 2 UTF-16 encoding units.

We have been recommending people use MultiByteToWideChar or WideCharToMultiByte instead and indeed that's what we use:

https://github.com/microsoft/STL/blob/ef62d3fa0b8e4e2406b9bb74e916e1ca8a1df802/stl/src/filesystem.cpp#L287-L316

[Febbe]

Just to be curious, couldn't you just provide both. The original implementation with the original internal namespace and a new one which is reachable via std::wstring_convert but has a slightly different internal inlined namespace?

That would preserve abi compatibility and fixes the issue for every newly compiled binaries. Although this is probably wasted effort for this particular case since it's deprecated in anyway.