Assertion failure on Delete[]

Category: visual studio parallelcpp

Question

ProgrammingInCSharp12453 on Sun, 01 Jun 2014 07:58:16


When calling delete[] to release memory, a debug assertion failure occurs in the destructor:

I allocate an array, why is it illegal to delete as an array? (In debugging, Visual Studio sees clrs as a memory chunk of 1 item..it is allocated to 100 in testing code.)

The assertion failure is the following:

Expression: _BLOCK_TYPE_IS_VALID(pHead->nBlockUse)

Gradient::Gradient()
{
	clrs = NULL;
	size = 0;
}

Gradient::~Gradient()
{ //Clrs is dynamic memory
	if (clrs != NULL)
	{
		delete[] clrs;
	}
}

void Gradient::Add_Color(SDL_Color clr, int duration)
{
	SDL_Color* t_clrs = NULL;
	//Todo: Add mixing?
	if (size > 0)
	{
		 t_clrs = new SDL_Color[size];
		for (int i = 0; i < size; i++)
		{
			*(t_clrs + i) = *(clrs + i);
		}
		delete[] clrs;
	}
	clrs = new SDL_Color[size + duration];
	if (size > 0)
	{
		for (int i = 0; i < size; i++)
		{
			*(clrs + i) = *(t_clrs + i);
		}
		delete[] t_clrs;
	}
	for (int i = 0; i < duration; i++)
	{
		*(clrs + i + size) = clr;
	}
	size += duration;
}
Note: This occurs when the program exits out.

Replies

davewilk on Sun, 01 Jun 2014 09:53:55


There are various uses of delete [] in this program. Are you sure which one is causing the problem?

Your program is unnecessarily complicated. In modern C++, any client use of new or new [] should be replaced by smart pointers. For new [] this normally means use std::vector.

Your whole code could then be simplified to

// untested
class Gradient { private: std::vector<SDL_Color> clrs; public: void Add_Color(SDL_Color clr, int duration); }; void Gradient::Add_Color(SDL_Color clr, int duration) { clrs.resize(clrs.size() + duration, clr); }


ProgrammingInCSharp12453 on Sun, 01 Jun 2014 18:18:34


I would LOVE to use vectors in my DLL, unfortunately, they corrupted the memory heap in the process before any vector function was called. (I used explicit instantation of them).

I am sure of which delete is causing the problem. Could this be a compiler / debugger mistake?

davewilk on Sun, 01 Jun 2014 20:10:23


I would LOVE to use vectors in my DLL, unfortunately, they corrupted the memory heap in the process before any vector function was called. (I used explicit instantation of them).

I am sure of which delete is causing the problem. Could this be a compiler / debugger mistake?

You need to show us exactly what you did with std::vector to make it corrupt the heap.

You need to tell US which delete [] caused the failure.

If you want us to reproduce your problem you need to provide complete code.

The chances of your issue being a Visual C++ bug are extremely small.

ProgrammingInCSharp12453 on Sun, 01 Jun 2014 22:05:19


The delete in the destructor.

Here is the old instantation of vector, I referenced: http://support.microsoft.com/kb/168958/en-us

#pragma once //Header Guard
#ifdef ROB //For Building the DLL
#define ROB_API __declspec(dllexport) 
#define TEMPL
#else //For Using the DLL
#define ROB_API __declspec(dllimport) 
#define TEMPL extern
#endif

#include "SDL.h"

#include <math.h>

#include "Collision.h" //For Circle

#include <vector>
using std::vector;

TEMPL template class ROB_API vector < SDL_Color > ;
TEMPL template ROB_API vector<SDL_Color>::vector();

#ifdef __cplusplus
extern "C"{
#endif

	class ROB_API Gradient
	{
	public:
		Gradient();
		~Gradient();
		void Add_Color(SDL_Color clr , int duration);
		void Unpack_Color(SDL_Color clr);
		SDL_Color Grab(int index);
		int Get_Length();
	protected:
		//vector<SDL_Color> clrs;
		SDL_Color *clrs;
		int size;
	};

The assertion failure is in the destructor, here is the testing code (in a separate project):

int main( int argc, char* args[] )
{
	SDL_Event event;
	bool quit = false;
	Init();
	SDL_Window* screen = SDL_CreateWindow("Sample Template" , 0 , 0 , 200 , 200 , SDL_WINDOW_SHOWN);
	SDL_Renderer* ren = SDL_CreateRenderer(screen , -1 , SDL_RENDERER_ACCELERATED | SDL_RENDERER_PRESENTVSYNC);
	Uint32 format = SDL_GetWindowPixelFormat(screen);
	Gradient g;
	g.Add_Color(White, 30);
	g.Add_Color(Red, 30);
	g.Add_Color(Blue, 30);
	g.Add_Color(White, 10);
	SDL_Texture* test = RenderCircle_Filled(100, g, format, ren);
	while (quit == false)
	{
		
		while (SDL_PollEvent(&event) == 1)
		{
			switch(event.type)
			{
			case SDL_KEYDOWN:
				{
					if (event.key.keysym.sym == SDLK_ESCAPE)
					{
						quit = true;
					}
					break;
				}
			case SDL_QUIT:
				{
					break;
				}
			}
		} //End of Poll-Event Loop
		SDL_Rect rect = { 0, 0, 200, 200 };
		SDL_RenderClear(ren); //Clear the screen
		SDL_RenderCopy(ren, test, NULL, &rect);
		SDL_RenderPresent(ren); //Update the Screen
	} //Main Game loop End
	SDL_DestroyTexture(test);
	SDL_DestroyWindow(screen);
	SDL_DestroyRenderer(ren);
	Quit();
    return 0;    
} //Assertion Failure is Here

The picture draws properly from the function. Here is the rendering code:

SDL_Texture* RenderCircle_Filled(int radi, Gradient grad, Uint32 format, SDL_Renderer* ren)
{
	//Let's try the new method, the midpoint alg
	//Improving Alg by using portions of the midpoint alg.
	int bpp = 0;
	int dia = radi * 2;
	Uint32 r, g, b, a;
	SDL_PixelFormatEnumToMasks(format, &bpp, &r, &g, &b, &a);
	int offset = 0;
	SDL_Surface* surf = SDL_CreateRGBSurface(NULL, dia, dia, bpp, r, g, b, a);
	if (surf == NULL)
	{
		return NULL;
	}
	SDL_SetColorKey(surf, 1, SDL_MapRGB(surf->format, 254, 254, 254));
	Uint32* pixels = (Uint32*)surf->pixels;
	bool hit = false;
	Circle cir;
	cir.center = { radi, radi };
	cir.radius = radi;
	Uint32 n_clr;
	int index = 0; //Color index
	SDL_Color t;
	for (int y = 0; y <= radi * 2; y++)
	{
		for (int x = 0; x <= radi * 2; x++)
		{
			*(pixels + (y * surf->w) + x) = SDL_MapRGB(surf->format, 254, 254, 254);
		}
	}
	for (int y = 0; y <= radi; y++)
	{
		index = 0;
		hit = false;
		for (int x = 0; x <= radi; x++)
		{
			t = grad.Grab(index);
			index++;
			n_clr = SDL_MapRGB(surf->format, t.r, t.g, t.b);
			if ((Circlular_Collision(cir, { x, y }) == true) && (hit == false))
			{
				hit = true;
				offset = x;
			}
			//if (dia - offset == x)
			//{
			//	x = dia;
			//	continue;
			//}
			if (hit == true)
			{
				*(pixels + (y * surf->w) + x) = n_clr;
				*(pixels + (y * surf->w) + (dia - x)) = n_clr;
				*(pixels + ((dia - y) * surf->w) + x) = n_clr;
				*(pixels + ((dia - y) * surf->w) + (dia - x)) = n_clr;
				*(pixels + (x * surf->w) + y) = n_clr;
				*(pixels + (x * surf->w) + (dia - y)) = n_clr;
				*(pixels + ((dia - x) * surf->w) + y) = n_clr;
				*(pixels + ((dia - x) * surf->w) + (dia - y)) = n_clr;
			}

		}
	}
	surf->pixels = pixels;
	SDL_Texture* tex = SDL_CreateTextureFromSurface(ren, surf);
	SDL_FreeSurface(surf);
	return tex;
}

ProgrammingInCSharp12453 on Sun, 01 Jun 2014 22:06:58


I forgot, when I debugged the issue (with vectors) I found that the vector started with random data inside that corrupted the heap. 

davewilk on Mon, 02 Jun 2014 00:36:29


The delete in the destructor.

Here is the old instantation of vector, I referenced: http://support.microsoft.com/kb/168958/en-us

There is way too much code here to be useful, and I do not think it is a compilable example as it stands.

You never mentioned a DLL in your original post, and your last post does not use std::vector, so let's forget about these features for the moment.

What I meant by "complete code" is not your complete code, but just a simple compilable example that exhibits the problem. Code like this:

#include <stdlib.h>
#include <assert.h>

typedef int SDL_Color;

class Gradient
{
private:
	int size;
	SDL_Color* clrs;

public:
	Gradient();
	~Gradient();
	void Add_Color(SDL_Color clr, int duration);
	int Get_Length();

};

Gradient::Gradient()
{
	clrs = NULL;
	size = 0;
}

Gradient::~Gradient()
{ //Clrs is dynamic memory
	if (clrs != NULL)
	{
		delete[] clrs;
	}
}

void Gradient::Add_Color(SDL_Color clr, int duration)
{
	SDL_Color* t_clrs = NULL;
	//Todo: Add mixing?
	if (size > 0)
	{
		t_clrs = new SDL_Color[size];
		for (int i = 0; i < size; i++)
		{
			*(t_clrs + i) = *(clrs + i);
		}
		delete[] clrs;
	}
	clrs = new SDL_Color[size + duration];
	if (size > 0)
	{
		for (int i = 0; i < size; i++)
		{
			*(clrs + i) = *(t_clrs + i);
		}
		delete[] t_clrs;
	}
	for (int i = 0; i < duration; i++)
	{
		*(clrs + i + size) = clr;
	}
	size += duration;
}

int Gradient::Get_Length()
{
	return size;
}

int main()
{
	Gradient g;
SDL_Color White = 0; SDL_Color Red = 1; SDL_Color Blue = 2; g.Add_Color(White, 30); g.Add_Color(Red, 30); g.Add_Color(Blue, 30); g.Add_Color(White, 10);
assert(g.Get_Length() == 100); return 0; }

This code compiles and runs without errors.

So you need to figure out what is really different about your code.

ProgrammingInCSharp12453 on Tue, 03 Jun 2014 00:38:25


I figured it out. It was a small error that in the function that draws the circle.

SDL_Texture* RenderCircle_Filled(int radi, Gradient grad, Uint32 format, SDL_Renderer* ren)
{

The function copies the gradient (via a copy constructor), and when the function exits, the destructor is called to release the static memory. However, the main program still has a copy with the same memory address. Therefore, when it exits it attempts to delete the memory again causing an assertion failure.

Thank you for your help.

davewilk on Tue, 03 Jun 2014 10:27:21


I figured it out. It was a small error that in the function that draws the circle.

SDL_Texture* RenderCircle_Filled(int radi, Gradient grad, Uint32 format, SDL_Renderer* ren)
{

The function copies the gradient (via a copy constructor), and when the function exits, the destructor is called to release the static memory. However, the main program still has a copy with the same memory address. Therefore, when it exits it attempts to delete the memory again causing an assertion failure.

Thank you for your help.

Ah yes, you are passing the Gradient by value. There is no reason for this; objects should almost always be passed by reference (or better const  reference if you can).

Your Gradient class violates the Rule of Three

http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29

If you are not going to implement the copy constructor and assignment operator, then you should disable them by declaring them private and not implementing them (C++11 has better mechanisms for this).

Or even better, have your Gradient class use std::vector internally as I suggested. Then the default copy constructor, assignment operator and destructor will work correctly, and your code will be much simpler. It would still be better to pass Gradient by reference though.