Exception thrown at 0x0037A5C2 project.exe: 0xC0000005: Access violation writing location 0xDDDDDDDD at the end of the program

Question

I have encountered this runtime exception at the very end of the program by simply creating an instance of the specified class, so I presume the issue lies with either the constructor, copy constructor, copy assignment operator or destructor. I have read up on and followed the rule of three to the extent of my limited cpp knowledge.

Source.cpp

#include "Header.h"
#include <iostream>

using namespace std;


int main() {


string command = "CREATE TABLE table_name IF NOT EXISTS ((column_1_name,type,default_value), (column_2_name,type,default_value))";
string columns[20] = { "column_1_name,type,default_value", "column_1_name,type,default_value" };
string commandData[9] = { "table_name", "IF NOT EXISTS" };

CommCREATETABLE comm(command, columns, commandData, 2, 2);

}

Relevant code from Header.h

class CommCREATETABLE {
    string fullCommand = "";
    string* columns = nullptr;
    string* commandData = nullptr;
    string tableName = "";
    int nrOfColumns = 0;
    int nrOfElements = 0;
    bool valid = false;

Constructor:

CommCREATETABLE(string command, string* columns, string* commandData, int nrOfRows, int nrOfElements) {
        this->setNrOfColumns(nrOfRows);
        this->setNrOfElements(nrOfElements);
        this->setCommand(command);
        this->setColumns(columns);
        this->setCommandData(commandData);
        this->valid = checkInput(this->commandData, this->columns);
        this->setTableName(commandData[0]);
    }

Copy constructor, copy assignment operator, destructor:

CommCREATETABLE(const CommCREATETABLE& comm) {
        this->setNrOfColumns(comm.nrOfColumns);
        this->setNrOfElements(comm.nrOfElements);
        this->setCommand(comm.fullCommand);
        this->setColumns(comm.columns);
        this->setCommandData(comm.commandData);
        this->setTableName(comm.tableName);
        this->valid = comm.valid;
    }
    ~CommCREATETABLE() {
        if (this->columns != nullptr) {
            delete[] this->columns;
        }
        if (this->commandData != nullptr) {
            delete[] this->commandData;
        }
    }

    CommCREATETABLE& operator=(const CommCREATETABLE& comm) {
        this->setCommand(comm.fullCommand);
        this->setColumns(comm.columns);
        this->setCommandData(comm.commandData);
        this->setTableName(comm.tableName);
        this->setNrOfColumns(comm.nrOfColumns);
        this->setNrOfElements(comm.nrOfElements);
        this->valid = checkInput(this->commandData, this->columns);
        return *this;
    }

The only setters that deal with dynamic memory allocation are the following:

void setColumns(const string* columns) {
        if (this->nrOfColumns >= 0) {
            this->columns = new string[this->nrOfColumns];
            memcpy(this->columns, columns, this->nrOfColumns * sizeof(string));
        }
        else throw EmptyCommandException();
    }
    void setCommandData(const string* commandData) {
        if (this->nrOfElements >= 0) {
            this->commandData = new string[this->nrOfElements];
            memcpy(this->commandData, commandData, this->nrOfElements * sizeof(string));
        }
        else throw EmptyCommandException();
    }

0xDDDDDDDD in Visual Studio means you are dereferencing a pointer to already freed heap memory: [https://stackoverflow.com/questions/127386/in-visual-studio-c-what-are-the-memory-allocation-representations](https://stackoverflow.com/questions/127386/in-visual-studio-c-what-are-the-memory-allocation-representations) — drescherjm, Nov 23 '20 at 18:29
I would avoid using `memcpy` like you are now (last pair of methods). Internally the `string` class holds a pointer, what you are doing now is copying that pointer. This means that when the original string gets deleted the pointer is no longer valid. However, your class still holds that very same pointer. So when your class gets deleted it tries to delete that pointer again. That will clearly cause a problem. Instead, simply copy each string in the array with `=`, by looping over all the strings, that will resolve the problem. — Qubit, Nov 23 '20 at 18:32

score 2 · Accepted Answer · answered Nov 23 '20 at 18:45

At a quick glance I would say the issue is in your setColumns and setCommandData functions. (I might of course be wrong, I did not try to run the code you presented nor the changes I made -- so there might also be a typo somewhere.)

There you use memcpy to copy the strings into your class. However, internally a C++ string holds a pointer to the actual string, so using memcpy actually only copies that pointer. As a result, once the original string gets deleted, the pointer you copied into your class is no longer valid (as the memory has already been freed). As a result, once your class also gets deleted it attempts to delete memory that has already been freed. That is probably where your error comes from.

In fact, if you added lines to your program where you tried to manipulate your class (after the original input strings have already been deleted), the problem would present itself even sooner, as you would be accessing memory that has already been freed. This would lead to undefined behaviour, which typically ends with a crash at some point.

A quick fix would be to change the way you copy the data, by using = for each string (in that way copying the actual strings into a new location in memory, rather than copying the pointer).

void setColumns(const string* columns) {
    if (this->nrOfColumns > 0) { // Creating an array of size 0 is also not a good idea. 
        this->columns = new string[this->nrOfColumns];
        for (int i = 0; i < nrOfColumns; i++) { // You don't need this everywhere. 
            this->columns[i] = columns[i]; 
            // I don't think naming things the exact same way is good practice. 
        }
    }
    else throw EmptyCommandException();
}
void setCommandData(const string* commandData) {
    if (this->nrOfElements > 0) { // Creating an array of size 0 is also not a good idea. 
        this->commandData = new string[this->nrOfElements];
        for (int i = 0; i < nrOfElements; i++) { // You don't need this everywhere. 
            this->commandData[i] = commandData[i]; 
            // I don't think naming things the exact same way is good practice. 
        }
    }
    else throw EmptyCommandException();
}

Alternatively, if you want to avoid making copies you should look into move, but I would suggest against this for the time being, if you are still learning. You'll get there soon enough.

Generalization: `memcpy` can only be used if the object being copied is [Trivially Copyabvle](https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable) — user4581301, Nov 23 '20 at 18:51
Side Note: If allowed, [use `std::vector`](https://en.cppreference.com/w/cpp/container/vector) rather than dynamically allocated arrays. Saves time and usually a tonne of debugging. — user4581301, Nov 23 '20 at 18:53
Probably best to replace any `memcpy` with [`std::copy`](https://en.cppreference.com/w/cpp/algorithm/copy) instead of a `for` loop. You get correct behavior in this case and with trivially copyable types it's likely to compile to a fast `memmove`. — Blastfurnace, Nov 23 '20 at 19:17

Exception thrown at 0x0037A5C2 project.exe: 0xC0000005: Access violation writing location 0xDDDDDDDD at the end of the program

1 Answers1