`delete[]` on `int*` allocated with `new` gives malloc error

Question

Following my understanding of C++ convention, I have:

class BlockRepresentation : public FPRepresentation
{
    private:
        class Block
        {
            public:
                int id;
                int fpDimensions;
                int* position;        // pointers in question
                int* blockDimensions; // pointers in question

                ~Block();
        };
        std::vector<Block> all_blocks;

    public:
        BlockRepresentation( int count, int dimensions, int volumn[] );
        void AddBlock( int id, int position[], int dimensions[] );
        std::string ToGPL();
};

where new blocks are created in AddBlock:

void BlockRepresentation::AddBlock( int id, int position[], 
        int dimensions[] )
{
    Block newBlock;

    newBlock.id = id;
    newBlock.fpDimensions = fpDimensions;
    newBlock.position = new int[fpDimensions];        // pointers in question
    newBlock.blockDimensions = new int[fpDimensions]; // pointers in question

    for (int i = 0; i < fpDimensions; ++i)
    {
        newBlock.position[i] = position[i];
        newBlock.blockDimensions[i] = dimensions[i];
    }

    all_blocks.push_back( newBlock );
}

so I have the following destructor:

BlockRepresentation::Block::~Block()
{
    delete[] position;
    delete[] blockDimensions;
}

but then I get:

rep_tests(11039,0x7fff71390000) malloc: *** error for object 0x7fe4fad00240: pointer being freed was not allocated

Why should I not delete[] the 2 pointers here?

You're missing a copy constructor to satisfy the [rule of three](http://en.cppreference.com/w/cpp/language/rule_of_three). My guess is that the pointer isn't allocated because a copy's destructor `delete[]`ed it. — Sean Cline, Jun 18 '16 at 01:48
Interesting, sounds very reasonable from how my tests are using the objects, experimenting now and will get back with the result. — Cescante, Jun 18 '16 at 02:02
And for future reference, what do I do to make this a better question? I don't think deleting it on accounts of the downvotes is the best action? — Cescante, Jun 18 '16 at 02:03
@Cescante May I ask why you're not using `std::vector` for all your "dynamic arrays"? You're using it already, so why are you are not using it instead of `int *`? If you did that, then all of these issues go away. — PaulMcKenzie, Jun 18 '16 at 02:05
@PaulMcKenzie I thought I'd use a plain array because these blocks are created to keep track of blocks of arbitrary dimensions for input and output purposes. There are no complicated operations to take advantage of all the other features of vector. But now that I'm learning more about c++, the better designs may be: 1. use 2 vector to keep track of the positions and volumes separately and get rid of the Block inner class -OR- 2. use vectors everywhere as you suggested. — Cescante, Jun 18 '16 at 02:15
@Cescante The goal is to get the code to work first. By introducing pointers into your class, you're now responsible to make sure that the class has the correct copy semantics since instances are being placed in a vector, and your class lacks these characteristics because of the raw pointer usage. Instead you could have done something [like this](http://ideone.com/VvDbDh) and have the code working. — PaulMcKenzie, Jun 18 '16 at 02:21

score 1 · Accepted Answer · answered Jun 18 '16 at 02:05

As was pointed out in the comments, you violated the rule of three, and the violation is very obvious:

{
    Block newBlock;

    // snip

    all_blocks.push_back( newBlock );
}

When this function returns, the newBlock object goes out of scope, and its destructor will delete all the newed arrays.

But you push_back()ed this object. This constructs a copy of the object into the vector. Because your Block does not define a copy constructor, the default copy-constructor simply makes a copy of all the pointers to the newed arrays.

If you somehow manage to avoid dereferencing the no-longer valid pointers, or you survived that experience, you're not of the woods yet. That's because, when the vector gets destroyed, and its Blocks get destroyed, their destructors will, once again, attempt to delete the same newed arrays that were already deleted once before.

Instant crash.

score 1 · Answer 2 · edited May 23 '17 at 12:32

There is nothing wrong with your Block destructor. It is doing its job, which is releasing the memory that is pointed to by your two int * member variables. The problem is that the destructor is being called on the same pointer value multiple times, which results in a double-free error.

The entity that causes this is the std::vector<Block>, since a std::vector will make copies of your Block object, and your Block object is not safely copyable.

Since the member variables of Block that are pointers are position and blockDimensions, the most painless way to alleviate this issue is to use std::vector<int> instead of int *, as demonstrated by this sample program.

However, if you really wanted to use int *, you would need to implement a user-defined copy constructor. In addition, a user-defined assignment operator would complement the copy constructor. This is what is called the Rule of Three.

 #include <algorithm>
 //...
 class Block
 {
    public:
       int id;
       int fpDimensions;
       int *position;
       int *blockDimensions;

       Block() : position(nullptr), blockDimensions(nullptr),
                 id(0), fpDimensions(0) {}
      ~Block()
      {
          delete [] position;
          delete [] blockDimensions;
      }

      Block(const Block& rhs) : id(rhs.id), fpDimensions(rhs.fpDimensions),
                                position(new int[rhs.fpDimensions]),
                                blockDimensions(new int[rhs.fpDimensions])
      {
         std::copy(rhs.position, rhs.position + fpDimensions, position);
         std::copy(rhs.blockDimensions, rhs.blockDimensions + fpDimensions,
                   blockDimensions);
      }

      Block& operator=(const Block& rhs)
      {
         Block temp(rhs);
         std::swap(temp.position, position);
         std::swap(temp.blockDimensions, blockDimensions);
         std::swap(temp.id, id);
         std::swap(temp.fpDimensions, fpDimensions);
         return *this;
      }
  };

See the live sample here.

See all of the hoops we had to jump through to get the Block class to behave correctly when used within a std::vector, as opposed to simply using std::vector<int>?

`delete[]` on `int*` allocated with `new` gives malloc error

2 Answers2