0

I've found out that heap corruption error on delete[] is caused by writing somewhere where I didn't allocate anything (according to these posts: First post, Second post), the problem is though that even after trying to find the issue in this case, I am still unable to find it.

I am to create a simple database table where as a parameter of the function I get a row with data that has to correspond to the column data type.

So basically I created an array of pointers on other arrays (2d array) so that each array is row.

This is declaration for Table class:

class DLL_SPEC Table {
public:

    void insert(Object** row);

    void remove(int rowid);

    Iterator* select();

    void commit();

    void close();

    int getRowCount() const;

    FieldObject** getFields() const;

    int getFieldCount() const;

    void setName(std::string name) { _nameOfTable = name; };
    void setNumberOfFields(int numberOfFields) { _numberOfFields = numberOfFields; }
    void setNumberOfRows(int numberOfRows) { _numberOfRows = numberOfRows; }
    void setFields(FieldObject** fields) { _fields = fields; }

    Iterator* select(Condition* condition) { throw 0; }
    int findRowId(Condition* condition) { throw 0; }
    void update(Condition* condition, std::function<void(Object**)> callback) { throw 0; }
private:
    std::string _nameOfTable;
    int _numberOfFields;
    int _numberOfRows;
    FieldObject** _fields;

    Object*** _rowValues;
};

and here is the method for inserting that is causing the heap corruption error at delete[] tmpArray.

void Table::insert(Object** row)
{
    Object*** tmpArray = new Object**[_numberOfRows + 1];
    Object** checkedRow = new Object*[_numberOfFields];
    // extend the array by 1 new row
    for (size_t i = 0; i < _numberOfRows; i++)
    {
        tmpArray[i] = new Object*[_numberOfFields];
    }

    for (size_t i = 0; i < _numberOfRows; i++)
    {
        for (size_t j = 0; j < _numberOfFields; j++)
        {
            tmpArray[i][j] = _rowValues[i][j];
        }
    }
    // check if the new row contains correct values for table
    for (size_t i = 0; i < _numberOfFields; i++)
    {
        if (StringObject* v = dynamic_cast<StringObject*>(row[i]))
        {
            if (v->isType(_fields[i]->getType()))
            {
                checkedRow[i] = v;
            }
            continue;
        }
        if (IntObject* v = dynamic_cast<IntObject*>(row[i]))
        {
            if (v->isType(_fields[i]->getType()))
            {
                checkedRow[i] = v;
            }
            continue;
        }
        if (DoubleObject* v = dynamic_cast<DoubleObject*>(row[i]))
        {
            if (v->isType(_fields[i]->getType()))
            {
                checkedRow[i] = v;
            }
            continue;
        }
        throw std::invalid_argument("The fields of this table don't match with the data you want to enter.");
    }
    tmpArray[_numberOfRows + 1] = checkedRow;
    _numberOfRows++;
    _rowValues = tmpArray;

    delete[] tmpArray;
    delete[] checkedRow;
}
Antrophy
  • 29
  • 6
  • 2
    In C++ you really want to use Standard Library containers like `std::vector` or `std::array` and steer away from things like `new[]`. There's an alarming amount of `dynamic_cast` going on in here as well. Your `new[]` + loop `new[]` becomes `std::vector>` in this case. – tadman Nov 14 '19 at 01:33
  • And it's going to be faster to rewrite the whole thing using standard C++ containers, which correctly allocate and deallocate all the memory for you, then to track down the reason for the heap corruption in all of this. – Sam Varshavchik Nov 14 '19 at 01:35
  • It's far from clear what this code even does, or why making a copy of all that data is necessary in the first place for what seems to be some kind of "check", the function of which remains utterly mysterious. – tadman Nov 14 '19 at 01:36
  • @tadman Unfortunately I am not allowed to use such containers so I'm stuck with having to do it this way. Regarding the number of dynamic casts, I also feel like that's way too many dynamic casts, but I'll try to optimize it once it's working ^^ – Antrophy Nov 14 '19 at 01:37
  • You should probably describe a row structure that has properly typed fields and use that to store your row data in a `std::vector` structure, or perhaps a `std::map` if you need to look them up by name. This approach of jamming in arbitrary "Object" type pointers and hammering them with `dynamic_cast` to see which conversion succeeds goes heavily against the grain of what a C++ program should be doing. – tadman Nov 14 '19 at 01:38
  • 1
    If this is one of those "C++" courses where they prevent you from using containers then it's not going to teach you much about C++. `new[]` is something you rarely, if ever, have to use when doing C++ correctly. It should be taught as an addendum to the course for *special situations only*, not as how you're supposed to do it by default. – tadman Nov 14 '19 at 01:39
  • @Antrophy *Unfortunately I am not allowed to use such containers* -- The STL containers have been around for 21 years, since C++ was standardized -- it isn't a library that showed up a year or two ago. As the previous comment stated, your entire code goes against all good practices of C++. It's like running a marathon in dress shoes instead of running shoes. – PaulMcKenzie Nov 14 '19 at 01:40
  • @tadman that's exactly it, I heavily agree and my lecturer does too, unfortunately for me, he'll talk with the course guarantor after the semester is over, until then, we're forced to code without libraries. That being said, I know that with vector it would be way easier, for now though, I am stuck using "2d arrays". – Antrophy Nov 14 '19 at 01:42
  • 1
    If you must debug this code (rather than rewrite it), I suggest running it under valgrind, as valgrind can often detect out-of-bounds array reads/writes and/or double-frees. (and if you can't run valgrind, you can simulate it, sort of, by adding print statements that print the value of every pointer you allocate and every pointer you free, and then you can grovel over the stdout output post-mortem to verify that every allocated pointer is freed exactly once... not fun, but doable in a pinch) – Jeremy Friesner Nov 14 '19 at 01:44
  • 1
    I feel for you. Personally I'd try and survive this course but I wouldn't count on it to teach you anything about what C++ is. There's a number of books on "Effective C++" that explains what you're supposed to do in C++, and most of it involves avoiding pointers, allocations with `new` or `new[]` primarily by implementing lean and efficient copy and move constructors for things you need to pass around or retain. – tadman Nov 14 '19 at 01:45
  • @Antrophy [See this](https://stackoverflow.com/questions/21943621/how-to-create-a-contiguous-2d-array-in-c/21944048#21944048). Hopefully that will encapsulate some of the 2D array stuff you're doing now. That link also describes how the way you've been taught in creating such a 2D array is the worst way to do it if all the rows are the same size. So even given the course's zeal in teaching you this way, they're teaching dynamic 2D array creation horribly. – PaulMcKenzie Nov 14 '19 at 01:46
  • C++ "without libraries" should not mean without the *Standard Library that comes with it*. It's like insisting you learn C without being able to `#include` anything. Good luck with that. – tadman Nov 14 '19 at 01:47
  • 1
    There is nothing stopping you from creating your own containers that follow the [rule of 3](https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)) and encapsulate all the memory handling. Also try using static analysis tools like cppcheck to find suspect code. That triple pointer is pretty funky and way hard to reason about, there must be a way to avoid using it. – Paul Rooney Nov 14 '19 at 01:55

1 Answers1

1

You allocate some space for tmpArray with

Object*** tmpArray = new Object**[_numberOfRows + 1];

at the end of the function you write to

tmpArray[_numberOfRows + 1] = checkedRow;

which writes past the end of the allocated space (subscripts run from 0 to _numberOfRows) resulting in Undefined Behavior.

Also, even if it didn't crash here, it would later because you assign tmpArray to _rowValues then delete tmpArray, leaving _rowValues dangling resulting in future issues when you dereference that pointer.

1201ProgramAlarm
  • 32,384
  • 7
  • 42
  • 56