Execption thrown when deleting a valid pointer

[noahpereira20]

Hi Fellow Programmers!

I am having some trouble with this small project.

I am getting a exception thrown at me from "delete_scalar.cpp:Line 34: _free_dbg(block, _UNKNOWN_BLOCK);"

Which stems from me trying to delete a pointer holding a char array.

Here is context

The Destructor

	Menu::~Menu() {
		//Deallocate title
		if (m_title) {
			delete[] m_title;
		}
		m_title = nullptr;

		//Deallocate menu items
		if (m_menuCount > 0) {
			for (int i = 0; i < m_menuCount; i++) {
				delete m_menuItem[i];
				m_menuItem[i] = nullptr;
			}
		}


		//Reset values
		m_menuCount = 0;
		m_indentation = 0;

		m_safe = true;

	}

Here is the declarations of the objects

   Menu mainMenu("** Main Menu **");
   Menu subMenu1("** Sub Menu One **", 1);
   Menu subMenu2("** Sub Menu **", 2);
   Menu tempMenu("** Temp **");
   Menu invMenu("** To test Invalid Menu **");

When the client code completes and the destructors start deleting the pointers, once the destructor hits mainMenu it throws the exception, the first four objects exit fine.

Here is the values right at return 0, before destructors get called.

-		invMenu	{m_title=0x00925288 "" m_menuItem=0x0073fd20 {0x00000000 <NULL>, 0x00000000 <NULL>, 0x00000000 <NULL>, ...} ...}	sdds::Menu
+		m_title	0x00925288 ""	char *
+		m_menuItem	0x0073fd20 {0x00000000 <NULL>, 0x00000000 <NULL>, 0x00000000 <NULL>, 0x00000000 <NULL>, 0x00000000 <NULL>, ...}	sdds::MenuItem *[11]
		m_menuCount	0	int
		m_indentation	0	int
		m_safe	true	bool
-		mainMenu	{m_title=0x00929b80 "" m_menuItem=0x0073fe30 {0x00931cd0 {m_item=0xdddddddd <Error reading characters of string.> }, ...} ...}	sdds::Menu
+		m_title	0x00929b80 ""	char *
+		m_menuItem	0x0073fe30 {0x00931cd0 {m_item=0xdddddddd <Error reading characters of string.> }, 0x00931f40 {m_item=...}, ...}	sdds::MenuItem *[11]
		m_menuCount	0	int
		m_indentation	0	int
		m_safe	true	bool
-		subMenu1	{m_title=0x00931f70 "" m_menuItem=0x0073fdec {0x00931df0 {m_item=0xdddddddd <Error reading characters of string.> }, ...} ...}	sdds::Menu
+		m_title	0x00931f70 ""	char *
+		m_menuItem	0x0073fdec {0x00931df0 {m_item=0xdddddddd <Error reading characters of string.> }, 0x00931e80 {m_item=...}, ...}	sdds::MenuItem *[11]
		m_menuCount	0	int
		m_indentation	0	int
		m_safe	true	bool
-		subMenu2	{m_title=0x00925248 "** Sub Menu **" m_menuItem=0x0073fda8 {0x00931d90 {m_item=0x0092e1d0 "The first" }, ...} ...}	sdds::Menu
+		m_title	0x00925248 "** Sub Menu **"	char *
+		m_menuItem	0x0073fda8 {0x00931d90 {m_item=0x0092e1d0 "The first" }, 0x00931fd0 {m_item=0x0092e320 "The second" }, ...}	sdds::MenuItem *[11]
		m_menuCount	3	int
		m_indentation	2	int
		m_safe	false	bool
-		tempMenu	{m_title=0x00933278 "** Sub Menu **" m_menuItem=0x0073fd64 {0x00931c40 {m_item=0x0092e208 "The first" }, ...} ...}	sdds::Menu
+		m_title	0x00933278 "** Sub Menu **"	char *
+		m_menuItem	0x0073fd64 {0x00931c40 {m_item=0x0092e208 "The first" }, 0x00931dc0 {m_item=0x0092e2b0 "The second" }, ...}	sdds::MenuItem *[11]
		m_menuCount	3	int
		m_indentation	2	int
		m_safe	false	bool

It's confusing me, weird error aswell oh and heres the error as seen from my eyes.

Debug Assertion Failed!

Program:
....myFiles\Project.exe
File:minkernel\crts\ucrt\src\appcrt\heap\debug_heap.cpp
Line: 904

Expression: _CrtIsValidHeapPointer(block)

P.S does memory management in c++ ever become second nature to you oldies asking from a starter in college

[mbozzi]

Good luck finding a programmer who can correct a memory problem without reading the source code.

There's no evidence of any exception being thrown in what you posted.
Post a small, self-contained, compilable example of the problem:
http://www.sscce.org/

P.S does memory management in c++ ever become second nature to you oldies asking from a starter in college

It becomes natural when you stop writing new everywhere.

[Ganado]

Show how you construct the objects. Show the places you call new and new[].

Also, setting a pointer to null in a destructor is probably useless because the object the pointer belongs to is about to not exist anyway.
Also also, your check on line 9 is redundant.
So is your check on line 3.

P.S does memory management in c++ ever become second nature to you oldies asking from a starter in college

If you don't use new & delete, you don't need to worry about "classic" memory management, although should always be conscious of what is happening in your code.

[noahpereira20]

Here is the reproducable example, tried to keep it simple.

#define _CRT_SECURE_NO_WARNINGS

#include <cstring>
#include <iostream>

class tempMenu {
private:
	char* m_title;
public:
	tempMenu(const char* title) {
		//Check title, allocate size for array, copy into array
		if (title && title[0] != '\0') {
			m_title = new char[strlen(title) + 1];
			strcpy(m_title, title);
		}
		else {	//else set to nullptr
			m_title = nullptr;
		}
	}

	~tempMenu() {
		//Deallocate, this is where it breaks
		delete[] m_title;
	}

	void display() {
		std::cout << m_title << std::endl;
	}
};


//Display menu
void testMenu(tempMenu temp) {
	temp.display();
}

int main(){
	
	//Create object
	tempMenu obj("Object");

	//Gets passed into new scope
	testMenu(obj);

	//Returns from scope after going throught destructor, object is essentialy destroyed here

	return 0;	//calls obj destructor, triggers error at delete [] m_title in destructor

}

I have to keep it passed by value.

Here is the client code with values before and after being passed into the new scope

   testMenus(mainMenu, subMenu1, subMenu2);

   void testMenus(Menu m, Menu sub1, const Menu& sub2)

Here are the values before going in...

-		mainMenu	{m_title=0x01591068 "** Main Menu **" m_menuItem=0x012ff794 {0x015924e8 {m_item=0x0158e0f0 "Option one" }, ...} ...}	sdds::Menu
-		m_title	0x01591068 "** Main Menu **"	char *
			42 '*'	char
-		subMenu1	{m_title=0x01590ca8 "** Sub Menu One **" m_menuItem=0x012ff750 {0x01592488 {m_item=0x01590f28 "Selection one" }, ...} ...}	sdds::Menu
-		m_title	0x01590ca8 "** Sub Menu One **"	char *
			42 '*'	char
-		subMenu2	{m_title=0x01590ea8 "** Sub Menu **" m_menuItem=0x012ff70c {0x01592638 {m_item=0x0158e1d0 "The first" }, ...} ...}	sdds::Menu
-		m_title	0x01590ea8 "** Sub Menu **"	char *
			42 '*'	char

and after leaving scope back to client code....

-		mainMenu	{m_title=0x01591068 "ÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝ" m_menuItem=0x012ff794 {0x015924e8 {m_item=0xdddddddd <Error reading characters of string.> }, ...} ...}	sdds::Menu
+		m_title	0x01591068 "ÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝ"	char *
-		subMenu1	{m_title=0x01590ca8 "ÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝ" m_menuItem=0x012ff750 {0x01592488 {m_item=0xdddddddd <Error reading characters of string.> }, ...} ...}	sdds::Menu
+		m_title	0x01590ca8 "ÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝÝ"	char *
-		subMenu2	{m_title=0x01590ea8 "** Sub Menu **" m_menuItem=0x012ff70c {0x01592638 {m_item=0x0158e1d0 "The first" }, ...} ...}	sdds::Menu
+		m_title	0x01590ea8 "** Sub Menu **"	char *

mainMenu and subMenu1 both get deleted, but mainMenu is the one that crashes when being destructed, subMenu1 destructs before mainMenu...

[mbozzi]

Let's reduce the problem more:

struct a
{ 
  int* ptr_ = nullptr;
  ~a() { delete ptr_; }
};

int main()
{
  a x { new int };
  a y = x;
}

See:
http://coliru.stacked-crooked.com/a/b06d48900f33a4bd

After line 10, both x.ptr_ and y.ptr_ point to the same object.

Destruction occurs in reverse order of construction. When main returns, y.~a is called, deleting y.ptr_. Next, x.~a is called, deleting x.ptr_. Unfortunately, x.ptr_ and y.ptr_ have the same value. The source of the problem is the attempt to delete the same pointer twice.

In the above program, we copy x into y on line 10, and this is the source of the problem. The copy in your code is harder to spot, but it occurs on line 44.

To fix the problem, either
a.) disallow copies; or
b.) duplicate the resource.

Case a.) is the simplest option, but it isn't always suitable. We add the definitions

class tempMenu 
{
public:
  // disallow initialization of one tempMenu from another
  tempMenu(tempMenu const&) = delete; 
  // disallow initialization of one tempMenu from a "temporary" tempMenu
  tempMenu(tempMenu &&) = delete;
  // disallow assignment of this tempMenu from another
  tempMenu& operator=(tempMenu const&) = delete;
  // disallow assignment of this tempMenu from a "temporary" tempMenu
  tempMenu& operator=(tempMenu &&) = delete;
};

to our code. Now we are forbidden to copy or assign tempMenu objects; attempts to do so will result in a compiler error, and not an error that results in wrong behavior on the user's machine.

We can still pass tempMenu objects as function arguments by binding them to reference parameters:

void testMenu(tempMenu /*const*/& temp) 
{
	temp.display();
}

[noahpereira20]

How do we duplicate the resource, my module uses the functionality of

Menu& operator=(const Menu& menu);

to assign menu's to one another

[mbozzi]

Add the definitions

tempMenu(tempMenu const& other)
  : m_title(new char[strlen(other.m_title) + 1]) 
{
  strcpy(m_title, other.m_title);    
}

tempMenu& operator=(tempMenu rhs) 
{
  swap(rhs);
  return *this;
}

void swap(tempMenu& that) noexcept
{
  using std::swap;
  swap(this->m_title, that.m_title);
}

to tempMenu

Look up the "copy-and-swap" idiom for more information about the copy assignment operator. The copy constructor is the more problematic of the two, but it should do for now.