Unstable behavior of dynamically allocated LPTSTR (TCHAR*) and delete[] operator?

Question

I was trying to build a class that deal with String operators. However, for no convincing reason, it sometime crashed during delete[] operator. I used strsafe library to do all internal string operation.

//qstring

LPTSTR m_string;

void QString::operator +=(const QString &_in) //Concat m_string with _in.m_string
{
    size_t size = strlength(m_string) + strlength(_in.m_string) +1; //new size
    LPTSTR buffer = new TCHAR[size]; //alloc buffer
    ::StringCchCopy(buffer,strlength(m_string)+1,m_string); //copy current m_string to buffer

    ::StringCchCat(buffer, size, _in.m_string); //concat buffer with the input

    Replace(buffer); //replace this object with m_string

    delete[] buffer; //dealloc
}


void QString::Replace(LPCTSTR src) //replace m_string with src
{
    size_t size = strlength(src)+1; //new size
    Alloc(size);
    ::StringCchCopy(m_string,size,src); //copy src to m_string
}


void QString::Alloc(size_t size) //Dynamic allocation
{
    if(m_string != NULL) Free();
    m_string = new TCHAR[size+1];
}


void QString::Free()  //Free m_string
{
        delete[] m_string; //Sometime crashes here
        m_string = NULL;
}


QString ToStr(int _in) //Convert Integer to qstring
{
    int size = 1;
    int f = _in;

    while(f > 0)
    {
        f /=10;
        size++;
    }

    TCHAR* buf = new TCHAR[size];

    for(int i = 0; i < size; i++) buf[i] = (TCHAR)TEXT("");

    QString result(L"undef");

    if(::_itow_s(_in,buf,size,10) == 0) //No error code = ok 
    {
        result = buf;
    }
    delete[] buf;
    return result;
}

//Example 1: Does not crashed

int ::WinMain(HINSTANCE hInst, HINSTANCE hPrevInst, LPSTR lpCmdLine, int nShowCmd)
{
    QString a(L"");
    a += L"TEST";
    a += ToStr(1000);

::MessageBox(0,a.GetStr(),L"NOTHING",MB_OK);

        return 0;
}

//Example 2: Print weird characters plus some of the current characters (Unicode problems?)

 int ::WinMain(HINSTANCE hInst, HINSTANCE hPrevInst, LPSTR lpCmdLine, int nShowCmd)
{
    QString a(L"");
    a += L"TESTTESTESTEST";
    a += ToStr(1000);
    ::MessageBox(0,a.GetStr(),L"NOTHING",MB_OK);

    return 0;
}

//Example 3: Crashes on load

int ::WinMain(HINSTANCE hInst, HINSTANCE hPrevInst, LPSTR lpCmdLine, int nShowCmd)
{
    QString a(L"");
    a += L"TESTTESTESTEST";
    a += ToStr(1000);
    a += L"TESTESTEST";
    a += ToStr(100);
    ::MessageBox(0,a.GetStr(),L"NOTHING",MB_OK);

    return 0;
}

It crashed on delete[] operator in Free(). With error of either

HEAP[StringTest.exe]: Invalid Address specified to RtlFreeHeap( 003C0000, 003C46A8 )

or

HEAP[StringTest.exe]: Heap block at 003C4750 modified at 003C475C past requested size of 4

Answer 1

You constructor doesn't initialize m_string to NULL (according to your own comment above). This is causing random Free() failures.

As selbie notes, there are other bugs and inefficiencies in this code. For example, Replace always reallocates, even when called from += that has already allocated. And you add 1 in both Replace and Alloc, suggesting you are not clear about which functions take values that include the terminator and which don't.

Unless this is a learning or homework exercise, I strongly recommend against writing your own string class - it will take you a lot more work to get something reasonable and efficient than it would to use std::string (or ATL::CString if you prefer).

Martyn

Answer 2

Another answer:

My psychic powers tells me that if QString doesn't have a copy constructor, then bad things are likely happening when ToStr() returns a "copy" of a QString on the stack.

Answer 3

I see lots of bugs.

For starters, if you pass in 0 to the ToStr function, you will only allocated one char of memory to hold the string "0" which is two bytes ('0' + null char). Further, if you pass a negative integer into this function, other bad things will happen. You are better off just allocating more than you need without trying to compute the "exact" size.

int size = sizeof(_in) * 4 + 1;

None of your strcpy's (even with using the StringCch functions) make me feel good.

I would suggest allocating 2x the amount of "size" you need, then carefully study the memory address of buffer and m_string via debugging if the strcpy goes beyond "size-1" bytes (not including the null terminating char).

Answer 4

your whole Allocation and free algorithms are predicated on the fact that m_string has been initialized to NULL. Have you done that in your constructor (I don't see it here)?

by the way, you may have a conflict with a CRT library function named Alloc(). http://msdn.microsoft.com/en-us/library/dd492420%28VS.100%29.aspx

your algorithm for calculating the length of an integer string is incorrect. you start out with size=1, you should start out with size=0. also, you need to handle the special case where 0==f: 0 is definitely a number and takes up 1 character. you had an off-by-1 error (fencepost error) where the size of the integer was always 1 too big.

QString ToStr(int _in) //Convert Integer to qstring
{
    int size = 0;
    int f = _in;

    if (0==f) {
        size=1;
    } else {
        while(f > 0) {
            f /=10;
            size++;
        }
    }

in Replace(), you Alloc() 2 extra characters.

size_t size = strlength(src)+1;

should be

size_t size = strlength(src);

and if you need an extra character for the ::StringCchCopy(m_string,size,src); put in size+1 there. or else change your Alloc so it allocates exactly the number of characters you ask for (like the name implies).