Suspicious memory leak in std::basic_string


A customer asked for assistance debugging a memory leak. Their memory leak detection tool was reporting a leak in the following call stack:

ntdll!RtlAllocateHeap
Contoso!malloc
Contoso!Allocator<unsigned short>::allocate
Contoso!std::basic_string<unsigned short,std::char_traits<unsigned short>,Allocator<unsigned short>,_STL70>::_Copy
Contoso!std::basic_string<unsigned short,std::char_traits<unsigned short>,Allocator<unsigned short>,_STL70>::_Grow
Contoso!std::basic_string<unsigned short,std::char_traits<unsigned short>,Allocator<unsigned short>,_STL70>::assign
Contoso!std::basic_string<unsigned short,std::char_traits<unsigned short>,Allocator<unsigned short>,_STL70>::assign
Contoso!std::basic_string<unsigned short,std::char_traits<unsigned short>,Allocator<unsigned short>,_STL70>::operator=
Contoso!ConfigurationImpl::validate

"The Configuration­Impl object itself is not being leaked. Just the string inside it."

The Visual C++ team reported that there are no known memory leaks in STL70. However, the code above is using a custom allocator, so they asked to see more of the customer's code.

And they found the smoking gun, but it wasn't in the allocator. It was in the class constructor.

ConfigurationImpl::ConfigurationImpl()
{
    // Initialize all members to zero.
    memset(this, 0, sizeof(ConfigurationImpl));
}

basic_string, like all STL objects, is non-POD. A POD type is roughly¹ something that can be declared in C as a plain old struct, such as struct Pod { int x; int y; };. POD types can be treated as a blob of bytes that you can manipulate with memset, memcpy, and such. Non-POD types, on the other hand, are those with things like constructors, destructors, virtual methods, all that fancy C++ stuff. You cannot treat them as just a blob of bytes because they have other fancy behaviors attached, and treating them as a blob of bytes bypasses (and may even damage) those fancy behaviors.

In this case, using memset to zero out a basic_string wipes out all the work that was performed by the basic_string constructor and results in the dreaded undefined behavior. Maybe undefined behavior manifests itself as a memory leak. Maybe it manifests itself as a crash. Maybe it manifests itself as time travel.

In practical terms, what you have there is memory corruption. When you have memory corruption, crazy things can happen. So don't corrupt memory.

The customer thanked us for our assistance and fixed their code.

¹This is a simplified discussion, so don't haul out your language-lawyer pitchforks.

Comments (15)
  1. Dilip says:

    Raymond -- you will have to forgive me here. I have no other means of communicating with you. So once again apologies for the tangent. This is regarding my comment here:
    I never got to see your follow up. An update: This bug was indeed communicated to secure@microsoft.com and to the best of my knowledge it has also been published out in the wild. I won't post the link here. If you are interested or curious, feel free to leave a comment here and I can send you the link offline.

    1. Joshua says:

      Raymond's email address is a puzzle. I solved it. You can too.

    2. Yes, I saw your communication with the security team. I am disappointed that you decided to publish your blog entry describing the vulnerability before the fix was released.

      1. Dilip says:

        That wasn't me. It was my mentor. I won't recognize a kernel bug-check if it bit me on my tush.

  2. CarlD says:

    ...or perhaps that's just a typo in the post :)

  3. Yeah, that was a transcription error.

  4. Joshua says:

    Sadly, this blog is acting up again. I know I saw an on-topic comment earlier but now it's not there.

    1. People reported a typo in the article. I fixed the typo.

  5. Tim! says:

    :quietly puts pitchfork back:

  6. Scarlet Manuka says:

    Sadly, the method of time travel via undefined behaviour has proven difficult to engineer into a workable time machine.

  7. David Haim says:

    Just to be correct, you can use memcpy/memove on types which are trivially copyable, which may be a non-pod type, depends on the nature of you objects.

    1. Joshua says:

      Right, but not memset.

  8. Neil says:

    If I recall correctly, the less wrong way to do this is to override operator new for the class.

    1. Pierre B. says:

      To avoid people reading your comment and getting a misguided advice: no, the new operator is not related. The new operator is only meant to allocate memory, not initialize it, it provides raw bytes. Initialization is the job of the constructor.

      Given that it's almost certain that the std::string was a member variable not help by a pointer, the operator new was not involved anyway.

      Third, even if you provided an operator new, a sub-class could provide its own and not match your augmented expectations.

      If you need optimized initialization based on using memset, you have to use a POD as Raymond said (plain old data).

    2. Zan Lynx' says:

      I think you mean that in the class's new override use global new to get a buffer, pave it with memset() and return the pointer to continue with the rest of the object construction.

      Just wanted to try to clarify for those who haven't used new overrides much.

Comments are closed.

Skip to main content