Heap corruption when deleting a string

Here is my code:

std::string readString()
{
     int strLen = Read<int>();
     char* rawString = new char[strLen];
     Read(rawString, strLen);
     rawString[strLen] = '\0';
     std::string retVal(rawString);
     delete [] rawString;
     return retVal;
 }

The first line reads the length of the string.

The second line creates a new char array (c-string) with the string length

The third line reads the string (its reading it from a file)

The 4th line adds a NULL to the end.

The 5th line creates an std::string out of the c-string.

The 6th line deletes the c-string (HEAP CORRUPTION HAPPENS HERE)

The 7th line returns the string, but it never reaches this point because of an error.

On the 6th line I get a heap corruption error: CRT detected that the application wrote to memory after end of heap buffer.

My question may be ob开发者_StackOverflowvious, but why am I getting a heap corruption? When I create an std::string, it should copy the string, and I should be safe to delete the c-string.

Currently, I'm suspecting that std::string is trying to access the c-string after I delete it.

Any ideas?

You're accessing past the reserved bytes for your string. You reserved strLen characters, but put a \0 at the character strLen. Counting as C arrays from 0, character strLen is at position strLen + 1, so you're putting a value outside the reserved space for the string. You should reserve strLen + 1 in the second line of your main for your code to work.

Change:

char* rawString = new char[strLen];

to:

char* rawString = new char[strLen + 1];

int strLen = Read<int>() probably only returns the length of a non-null-terminated string, and when you try to write the \0 byte to the string, you run into buffer overflow problems.

You should check what strLen is, and most likely you either have to allocate like this:

char *rawString = new char[strlen+1];

Or use the overloaded constructor of std::string(const char *, size_t n) like this:

std::string retVal(rawString, strlen);

Since arrays are 0-indexed in c++, when you create an array of size strLen and then place a 0 at position strLen, you are writing that zero one after the end of the array you allocated.

Many advices so far, but none which address the exception safety issue: how do you get rid of that potential memory leak ?

There are two ways to avoid allocating with new (and thus facing a memory leak). The first is extremely simply and makes use of a compiler extension known as VLA for Variable Length Array:

std::string readString()
{
  int strLen = Read<int>();
  char rawString[strLen+1]; // VLA: the length is determined at runtime
                            // but the array is nonetheless on the stack
  Read(rawString, strLen);
  rawString[strLen] = '\0';

  std::string retVal(rawString);
  return retVal;
}

The other is compliant with the standard: string has an internal buffer which you can access (thanks to GMan, data is not the right access method)

std::string readString()
{
  int strLen = Read<int>();

  std::string retVal(strLen, '\0'); // no need to allocate extra space

  Read(&retVal[0], strLen);      // &retVal[0] gives access to the buffer

  return retVal;
}

I do believe that the last version is MUCH better. There is no longer any copying involved :)

 rawString[strLen] = '\0';

Writes the NUL off the end of the space you have allocated.

If strLen is 10, then you allocate space for 10 characters, read 10 characters, and write this NUL in position 11. Ooops