delete[] causing heap corruption

Question

I'm well aware that there are countless problems like this, but I searched for hours and couldn't understand what I did wrong so I would really appreciate your help. (I'm new to programming)

I need to create a dictionary manager of sorts as part of my homework but I seem to have a problem with deleting words. I get an error message "...triggered a breakpoint".

The usual answer people get to this problem is that this is heap corruption caused by going out of bounds but I can't see if and how I caused this.

I already made something similar with bus info management and it worked perfectly so that makes me even more confused... (Obviously, I did not make the mechanism exactly the same, but even after looking at my previous code I couldn't isolate the problem)

I added the functions I believe are of concern,

The adding function:

void Add_Word(char**& dictionary, int& dictionary_size, char word[])
{
    char** temp = new char*[dictionary_size + 1];   // Create a new array of appropriate size.

    int i;
    for (i = 0; i < dictionary_size; i++)
    {
        temp[i] = dictionary[i];    // Copy head pointers addresses for all existing items.
    }
    temp[i] = new char[strlen(word)];   // Add the space for the new word,
    temp[i][strlen(word)] = '\0';   // mark its end

    strcpy_s(temp[i], strlen(word) + 1, word);  // then copy it.
    // I'm really not so sure about what I should put in the buffer length but
    // strlen(word) + 1 seemed to work... I know... not good, but strlen(word) alone caused a problem.

    if (dictionary_size > 0)
        delete []dictionary;    // Delete previous head pointers array if there are any and
    dictionary = temp;  // reset the main pointer to the address of the new one.

    dictionary_size++;  // Finally, increase dictionary_size.
}

The deleting function:

void Delete_Word(char**& dictionary, int& dictionary_size, char* word)
{
    // !!! This is where the crash thingy happens.
    delete[] Search_For_Word(dictionary, dictionary_size, word);    // Delete the word from the dictionary.
    // Search_For_Word returns a pointer to the word it receives, from the dictionary.

    char** temp = new char*[dictionary_size - 1];   // Create a new array of appropriate size.

    int i;
    for (i = 0; i < dictionary_size; i++)
    {
        if (dictionary[i][0])
            temp[i] = dictionary[i];    // Copy the head pointers of the existing
           // items to the new array except for the deleted word.
    }

    delete[] dictionary;    // Delete previous head pointers array and
    dictionary = temp;  // reset the main pointer to the address of the new one.

    dictionary_size--;  // Finally, decrease dictionary_size.
}

EDIT: Any parts that are excessively inefficient or obviously broken are likely a result of me messing with my code trying to figure this out on my own (such as the calling 3 times to strlen mentioned (thanks again for that, kfsone...), or forgetting to +1 it for the '\0' to mark the end of a string --actually, no, if we go by obvious you won't tell me my mistakes @.@).

As for the reason I'm dealing with char instead of strings and vectors please allow me to quote myself: "...as part of my homework". I just barely started programming. That, and I want to grasp the basics before moving on to using the more comfortable higher-up tools.

Answer 1

The code is now working.

It was wrong all over. I messed up pretty much any part that I could regarding the dynamic memory while trying to fix it before.

I initially didn't care about calling 3 times to strlen becuase it's just homework and a very small program but I guess it's better to get used to do things the right way... I also dropped the copy which I evidently don't understand very well in favour of a simple for loop.

// Add function. The rest is cut.
    int word_length = strlen(word);

    temp[i] = new char[word_length + 1];    // Added +1 here.
    temp[i][word_length] = '\0';    /* This was correct after all.
    the word_length index is the correct ending.*/

    for (int j = 0; j < word_length; j++)   // copy replaced by for loop.
        temp[i][j] = word[j];
    // cut
}

void Delete_Word(char**& dictionary, int& dictionary_size, char* word)
{
    delete[] Search_For_Word(dictionary, dictionary_size, word);
   // There was a -1 mistake here I made in order to try and fix the thing earlier.
// No need for more, it works perfectly now.

Answer 2

Change:

temp[i] = new char[strlen(word)]

To:

temp[i] = new char[strlen(word)+1]

Answer 3

Your code has several problems.

First, if you want to allocate a C-style string on the heap using new[], then you must pay attention to the terminating NUL character.

So, if you want to do a deep copy from a string word, then you must calculate enough room, considering strlen(word) + 1: the +1 is for the terminating NUL character.
e.g.:

// Original code (wrong):
//
//     temp[i] = new char[strlen(word)];
//
// New code:
temp[i] = new char[strlen(word) + 1]; // consider terminating NUL (+1)

Moreover, following your code with explicit new[]s and delete[]s is not easy.
In modern C++, you may want to use convenient robust container classes like std::vector and string classes like std::string, instead of raw C-style pointers and strings.

You can simply store a list of strings using a std::vector<std::string>, and vector::push_back() method to add new strings to the vector. No need to complicate code with new[], delete[], strcpy_s(), etc.

And if you want to deep-copy strings, you can just use the simple natural overload of operator= for std::string, and copy constructors; e.g. std::string temp = word; will work just fine.

Answer 4

This is C++, why are you not using std::string instead of char buffers?

If you must use char buffer strings and the secure forms of strcpy_s know that the buffer length must always be the size of the destination buffer, never a strlen function. In your case it is a bit understandable since you created the buffer with the strlen function. But what you should do is set the value into a variable and then use that any time you need the buffer size.

Also, and where I think your bug is, you are writing temp[i][strlen(word)] = '\0'; But the actual indexes of the buffer go from 0 to strlen(word)-1 so you're writing outside the allocated memory.