Malloc error in std::string

Question

I'm getting the error

malloc: *** error for object 0x101346a70: pointer being freed was not allocated

When using an std::string. Sometimes it occurs, sometimes it does not.

 string1 += string2[i]

This is the statement which it breaks on (I've obviously changed the variable names)

I'm running Xcode 4.0.2 and using llvm.

I'm fairly certain that there isn't a bug with my code (although the error only occurs under a single function call). Is std::string even supposed to have errors like that?

Some googling returns this:

I haven't tried doing this however, as I use Macro's and the article is incredibly breif and not explanatory as to what macro causes this issue and why. I also found some other logs of mac developers complaining of similar errors, but have sofar found no other solutions.

Do I just have buggy code, or is this a problem with Xcode/LLVM/STL Implementation

-- Edit for source code and class explinations --

Explanations:

This function is for getting a c-string out of a class which is message, primarily a string and some accompanied data. A printf style notation is used for where the data should go in the message, ie %f means float.

MSIterator is just a typecast for an unsigned int.

I should point out that I wrote this a few years ago, so it doesn't have the best practises in terms of naming and other practises.

Hopefully the variable names will be all thats needed to show what the variables are.

string MSMessage::getString()
{
    string returnValue;

    Uint stringElementIndex = 0;
    Uint floatElementIndex = 0;

    // Iterate through arguments
    for(MSIterator messageIndex = 0; messageIndex < m_message.size();++messageIndex)
    {
        if(m_message[messageIndex] == '%')
        {
            // Check the next character
            switch (m_message[++messageIndex]) 
            {
                case 's':                                                           
                    returnValue += string(m_stringArguments[stringElementIndex]);
                    ++stringElementIndex;                   

                    break;

                case 'f':
                    returnValue += StringServices::toString(m_floatArguments[floatElementIndex]);
                    ++floatElementIndex;

                    break;

                case '%':

                    returnValue+='%';

                    break;

                default:

                    /* Otherwise, act as if its normal text */

                    returnValue+='%';
                    returnValue+=m_message[messageIndex];

                    break;
            }
        }

        // Not a argument? Tack it on!
        else 
        {
                 // Malloc Error is here, with one character in returnValue and another 50 or so in m_message,
                 // Message index is correctly equal to 1
             returnValue += m_message[messageIndex];
        }
    }

    return returnValue;
}

m_message is set to "The text object iterator given to GUITextLine::insertTextObject was not valid". ie, there is no associated data, and no '%' characters to complicate things.

EDIT 2:

The function now returns an std::string (I've changed the source code as well), and still fails with the same error. I'm really confused.

EDIT 3:

This:

MSMessage x = MSMessage("The text object iterator given to GUITextLine::insertTextObject was not valid", MSRegion_Systems, MSZone_Object, MSType_Error_Orange, 0);

string y;

y =  x.getString();
y =  x.getString();
y =  x.getString();
y =  x.getString();
y =  x.getString();
y =  x.getString();
y =  x.getString();
y =  x.getString();
y =  x.getString();
y =  x.getString();

y =  x.getString();

Doesn't seem to be causing any problems. Any thoughts?

Answer 1

I believe the safest solution here would be to have the caller pass down the string that is currently declared as returnValue after having creating it using new(), have getString() (which might be better called stuffString()) stuff it and expect to caller to use it and free it using delete() when done with it. Any use of malloc() with a properly designed std class introduces un-necessary danger, since the class can only keep track of the storage it has allocated for itself and of course, that which was allocated by new(). I belive that once created, std:string should automatically allocate and track any space needed for any text added to it using its own methods and overloaded operators (if I'm being naive here, please correct me), so there should be no need to ever use malloc().

PS: The one exception I can see to this is the case where one wants to instantiate a C-style string with the contents of an existing std:string. Then one could write

char* new_C_string = malloc(strlen(old_CPP_string.c_str) + 1);
strcpy(new_c_string, old_CPP_string.c_str);

This would, of course, be a last resort to glue existing C code with existing C++ code. Tomas it right, this stuff can get very confusing.

Answer 2

return returnValue.c_str();

You may not do this. .c_str() necessarily returns a pointer to the string's internal storage, because there is no way to have it create a copy without there being a memory leak in general. (This is not C, where there is a culture of waving a magic documentation wand and thereafter freely expecting the caller to clean up the callee's messes.) Upon returning from the function, returnValue ceases to exist, and thus returnValue.c_str() is a dangling pointer, and any attempt to use it is undefined behaviour.

The normal way to approach the problem is to - drum roll - just return the string. Use the string class everywhere possible - why reject a real string type when you finally have it? The reason .c_str() is named that way is because it's for C interoperability. Accordingly, you use it at the point where actual interoperation with C code is required. For example, when passing the string to a C function.

Note that the C function may not legally modify the pointed-at data, because there is no way for the string object to know about the changes, so that would break the class invariants. If your C function requires a mutable char buffer, then you'll have to create one. One possible approach is to copy the characters into a std::vector<char> and pass a pointer to the internal storage of that. Of course, you still won't be able to capitalize on the vector's auto-resizing, because there is no way for the C code to interact with the vector's full interface. But then, most well-behaved C code doesn't assume it can "resize" strings "in-place" anyway. If you have something that expects a char** and the ability to replace the input "string", or something that attempts to realloc() a string, well... you may have to get a little creative, or better yet just abandon that C library.

Answer 3

returnValue is a local variable, so it automatically gets deleted when the function returns. This invalidates returnValue.c_str(). After returning to the calling function, the contents of the returned value will be undefined; and any attempt to free the pointer will corrupt the heap. This heap corruption may not be detected immediately; in fact it looks like it gets detected on a subsequent call of MSMessage::getCString().

Edited to add: As a quick fix, you can just change the last line to:

char* t = new char[returnValue.length() + 1] ;
strcpy (t, returnValue.c_str()) ;
return t ;

It is not good style to allocate something in a called function that must be freed by the calling function like this, but judging by your comment, it will save you a lot of work :-)

Answer 4

This kind of error is often caused by a memory corruption somewhere else in your program. The next call to one of the memory managment functions will then fail, because the heap is corrupt.