3

I start with a very simple program:

#include <TBString.h>

int main(int argv, char** argc)
{
    tb::String test("");
    test = "Hello World!";

    return 0;
}

tb::String is my own string class, which was designed to handle both char strings and wchar_t (Unicode) strings. It is heavily templated, tb::String is a typedef of tb::StringBase<char>.

The whole thing is compiled using the CRT debugging utilities to check for memory leaks. Here's the output:

Detected memory leaks!
Dumping objects ->
c:\users\sam\svn\dependencies\toolbox\headers\tbstring2.inl(38) : {442} normal block at 0x00D78290, 1 bytes long.
 Data: < > 00 
{131} normal block at 0x00C5EFA0, 52 bytes long.
 Data: <                > A0 EF C5 00 A0 EF C5 00 A0 EF C5 00 CD CD CD CD 
Object dump complete.
Detected memory leaks!
Dumping objects ->
c:\users\sam\svn\dependencies\toolbox\headers\tbstring2.inl(38) : {442} normal block at 0x00D78290, 1 bytes long.
 Data: < > 00 
Object dump complete.
The program '[2888] SAM_release.exe: Native' has exited with code 0 (0x0).

So it looks like an empty tb::String (with size 0) is causing the memory leak. Confirmed with this program, which doesn't leak:

#include <TBString.h>

int main(int argv, char** argc)
{
    tb::String test("Hello World!");

    return 0;
}

Call stack for the original program:

  • Create a StringBase<char> with string "".
  • m_Length is set to 0.
  • m_Maximum is set to m_Length + 1 (1).
  • m_Data is created with a length of m_Maximum (1).
  • m_Data is cleared and filled with "".
  • _AppendSingle is set to StringBase<char>::_AppendDynSingle.
  • The overloaded operator StringBase<char>::operator = is called with string "Hello World!"
  • _AppendSingle is called.
  • m_Length is 0, m_Maximum is 1.
  • checklen is set to m_Length + src_len + 1 (13).
  • m_Maximum is multiplied by 2 until it is larger than checklen (16).
  • The StringBase<char>::Resize function is called with the new maximum.

Resize function:

template <typename C>
TB_INLINE StringBase<C>& StringBase<C>::Resize(int a_Maximum /*= -1*/)
{
    if (!m_Data)
    {
        m_Maximum = (a_Maximum == -1) ? 4 : a_Maximum;
        m_Data = new C[m_Maximum];
        StringHelper::Clear<C>(m_Data, m_Maximum);
    }
    else
    {
        int newmax = (a_Maximum == -1) ? (m_Maximum * 2) : a_Maximum;

        C* temp = new C[newmax];
        StringHelper::Clear<C>(temp, newmax);
        if (m_Length > 0) { StringHelper::Copy(temp, m_Data, m_Length); }
        delete [] m_Data;
        m_Data = temp;

        m_Maximum = newmax;
    }

    return *this;
}

This is what I suspect is the problem. Now, my question becomes:

How can I reallocate memory in C++ without it triggering a memory leak in the CRT debugger?

Constructor:

TB_INLINE StringBase<char>::StringBase(const char* a_String)
{
    m_Length = StringHelper::GetLength<char>(a_String);
    m_Maximum = m_Length + 1;
    m_Data = new char[m_Maximum];
    StringHelper::Clear<char>(m_Data, m_Maximum);

    StringHelper::Copy<char, char>(m_Data, a_String, m_Length);

    _AppendSingle = &StringBase<char>::_AppendDynSingle;
    _AppendDouble = &StringBase<char>::_AppendDynDouble;
}

Destructor:

TB_INLINE StringBase<char>::~StringBase()
{
    if (m_Data) { delete [] m_Data; }
}

Assignment operator:

TB_INLINE StringBase<char>& StringBase<char>::operator = (const char *a_String)
{
    Clear();
    return (this->*_AppendSingle)(a_String);
}

Append function:

template<>
TB_INLINE StringBase<char>& StringBase<char>::_AppendDynSingle(const char* a_String)
{
    if (!a_String) { return *this; }

    int src_len = StringHelper::GetLength<char>(a_String);

    // check size

    if (m_Maximum == -1)
    {
        m_Maximum = src_len + 1;
        m_Data = new char[m_Maximum];
        StringHelper::Clear<char>(m_Data, m_Maximum);
        m_Length = 0;
    }

    int checklen = m_Length + src_len + 1;
    if (checklen > m_Maximum)
    {
        while (checklen > m_Maximum) { m_Maximum *= 2; }
        Resize(m_Maximum);
    }

    // append

    strcat(m_Data, a_String);

    // new length

    m_Length += src_len;

    return *this;
}

Please note: I do not want to use std::string or std::vector, I want to fix this function.

knight666
  • 1,599
  • 3
  • 22
  • 38
  • 1
    That question seems to be mostly about the `tb::String` class. Is this one of your own now or from some library? – leftaroundabout Jun 12 '12 at 09:45
  • What is `tb::String`? Something serious, or something you hacked up? – Kerrek SB Jun 12 '12 at 09:46
  • `tb::String` is my own dynamic string class. I can post full source if required, but I believe I pasted the most relevant bits. – knight666 Jun 12 '12 at 09:47
  • I think I read somewhere that delete [] ptr; may have no effect. try just delete ptr; or free(ptr). – ActiveTrayPrntrTagDataStrDrvr Jun 12 '12 at 09:49
  • 3
    @user1240436 -1. Using `delete ptr` on an array causes a heap corruption. – knight666 Jun 12 '12 at 09:50
  • @knight666 The only relevant bits are the constructors, the destructor and the assignment operator. I don't see any of them posted. – James Kanze Jun 12 '12 at 09:50
  • You say that you don’t want to use ready-made tools, instead you want to solve your problem, yet those tools would do exactly that. I agree that it may be unsatisfactory but it’s the right thing to do. If you insist on using pointers (but let me tell you, that’s not a good idea), use lifetime-managing smart pointers. **There is simply no excuse for manually managing memory in your code.** It’s bad practice. – Konrad Rudolph Jun 12 '12 at 09:57
  • 2
    @KonradRudolph - There are many excuses for that: you are writing your own smart pointer class; you are using a pre C++98 compiler; you are writing an embedded (freestanding) application; or you just want to have some pointer-fun! – rodrigo Jun 12 '12 at 10:41
  • 2
    @rodrigo Read my comment again, I said “in your code”. I was specifically referring to OP’s use-case. I somwehat agree about embedded environments but this speaks against using `delete`, not as much against smart pointers. And pointer-fun isn’t a serious application. But even in general, I fail to see how those few edge cases qualify as “many excuses”. – Konrad Rudolph Jun 12 '12 at 10:46
  • @knight666 my bad memory. But I think the real problem with this code is that it's absolutely unclear what it does without sifting through all of it. – ActiveTrayPrntrTagDataStrDrvr Jun 12 '12 at 10:52
  • @user315052 Specialized all the functions just for you. `Clear` sets `m_Length` to 0 and fills the `m_Data` array with 0's. It, you know, clears the string. – knight666 Jun 12 '12 at 11:07
  • @user315052 Positive. `m_Maximum` is the variable that keeps track of the actual size of the array, while `m_Length` is the filled amount. Setting `m_Maximum` to -1 would not make sense, because then you'd lose information about the size of the array in memory. I apologize for coming off as sarcastic in my previous comment, your question was valid. – knight666 Jun 12 '12 at 11:12
  • @user315052 You are correct. If I fill the constructor with "Hello World!", the leak is 14 bytes. – knight666 Jun 12 '12 at 11:32
  • Writing a string class is hard. If you need to ask this kind of question, you shouldn't be doing it. Voting to close as "not helpful to other visitors." – Ben Jun 12 '12 at 11:58

2 Answers2

1

This is going to be a long one.

First, I decided to check my sanity. Does the CRT memory debugger work correctly?

int* src_test = new int[10];
for (int i = 0; i < 10; i++) { src_test[i] = i; }
int* dst_test = new int[10];
for (int i = 0; i < 10; i++) { dst_test[i] = src_test[i]; }
delete [] src_test;

This correctly reports a leak of 40 bytes. This line fixes the leak:

delete [] dst_test;

Okay, what else? Well, maybe the deconstructor is not being called. Let's put it in a function:

void ScopeTest()
{
    tb::String test("Hello World!");
    test = "Hello World! Again!";
}

It works, but it leaks. Let's make absolutely sure the deconstructor is called.

void ScopeTest()
{
    tb::String* test = new tb::String("Hello World!");
    *test = "Hello World! Again!";
    delete test;
}

Still leaking. Well, what does the = operator do? It clears and it appends. Let's do it manually:

void ScopeTest()
{
    tb::String* test = new tb::String("Hello World!");
    test->Clear();
    test->Append("Hello World! Again!");
    delete test;
}

Same result, so it has nothing to do with the operator. I wonder what would happen if I removed the Clear...

void ScopeTest()
{
    tb::String* test = new tb::String("Hello World!");
    test->Append("Hello World! Again!");
    delete test;
}

Alright, it... wait, what? It doesn't leak? What does Clear do then?

template <>
TB_INLINE StringBase<char>& StringBase<char>::Clear()
{
    if (m_Data)
    {
        StringHelper::Clear<char>(m_Data, m_Maximum);
    }

    m_Length = 0;

    return *this;
}

That's... harmless. But let's comment it out. 

template <>
TB_INLINE StringBase<char>& StringBase<char>::Clear()
{
    /*if (m_Data)
    {
        StringHelper::Clear<char>(m_Data, m_Maximum);
    }

    m_Length = 0;*/

    return *this;
}

Same result, no leaks. Let's remove the call to Clear again. 

void ScopeTest()
{
    tb::String* test = new tb::String("Hello World!");
    //test->Clear();
    test->Append("Hello World! Again!");
    delete test;
}

Leaking bytes again...

But wait a second, it's still clearing the tb::String? The length is set to 0 and the data is zeroed out, even though the body is commented out. How, what...

Alright, compiler, let's see you compile this:

/*template <>
TB_INLINE StringBase<char>& StringBase<char>::Clear()
{
    if (m_Data)
    {
        StringHelper::Clear<char>(m_Data, m_Maximum);
    }

    m_Length = 0;

    return *this;
}*/

Ha! That will show him! Oh wait... it... still compiles and runs.

Am I using a different version of the same file? No, I only have one version of TBString2.h and TBString2.inl on this computer...

Oh.

Oh wait a second.

Oh goddammit.

This better not be what I think it is.

Project Toolbox -> $(OutDir)\$(ProjectName)_d.lib

I'm going to murder the person who spent three hours on this.

Project Game -> Toolbox.lib

Oh wait. That was me.

TL;DR: I linked to an old build of the string class, causing all kinds of weird behavior, including leaking memory.

knight666
  • 1,599
  • 3
  • 22
  • 38
0

You are leaking whatever bytes were initialized in the constructor after you perform an assignment. To debug the leak, step through with the debugger when you perform the assignment. It might be useful to set a watchpoint on the m_Data variable so that the debugger will stop whenever it changes value.

jxh
  • 69,070
  • 8
  • 110
  • 193