c++stringvalgrindcopy-constructormemcheck

C++ - Valgrind: Invalid write of size 1


I have an AVL tree with string keys, and my own String class. In order to fix another issue, I had to add a copy constructor to String. However, valgrind reports an error with it. Here's the constructor:

String::String(const String& s){
    try {
        mStr = new char[1];
    }
    catch (const bad_alloc& e){
        printf("ERROR: No memory\n");
        exit(0);
    }
    mStr[0]='\0';
    mCopy(s);
}

void String::mCopy(const String& s){
    //  delete[] mStr;
    size_t n = s.Length();
    try{
        mStr = new char[n + 1];
    }
    catch (const bad_alloc& e){
        printf("ERROR: No memory\n");
        exit(0);
    }
    strcpy(mStr, s.mStr);
    mStr[n + 1] = '\0';
}

And here's a part of Valgrind output after adding a string key to my AVL tree:

==7005== Invalid write of size 1
==7005==    at 0x4013A1: String::mCopy(String const&) (in /home/ezrafell/Desktop/DA2final/DA3.o)
==7005==    by 0x401213: String::String(String const&) (in /home/ezrafell/Desktop/DA2final/DA3.o)
==7005==    by 0x40182D: main (in /home/ezrafell/Desktop/DA2final/DA3.o)
==7005==  Address 0x5ad450d is 0 bytes after a block of size 61 alloc'd
==7005==    at 0x4C2E80F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7005==    by 0x40136B: String::mCopy(String const&) (in /home/ezrafell/Desktop/DA2final/DA3.o)
==7005==    by 0x401213: String::String(String const&) (in /home/ezrafell/Desktop/DA2final/DA3.o)
==7005==    by 0x40182D: main (in /home/ezrafell/Desktop/DA2final/DA3.o)

Other errors reported by Valgrind also trace back to operator new[] in String::String(String const&). But what's wrong with it? And how do I rewrite the constructor to avoid this error?


Solution

  • The error is produced by this line:

    mStr[n + 1] = '\0';
    

    n+1 is one past the allocated size. You need to use n instead, because the body of the string is located at indexes 0 through n-1, inclusive, so the index n is where null terminator goes:

    mStr[n] = '\0';
    

    Note Removing this line will be correct, too, because strcpy null-terminates its result. Placing null terminator manually is required when you use functions that do not do it for you, such as memcpy.