Need Help with some memory leak issues

[noahpereira20]

Hi all, I am having trouble getting rid of some memory errors, I'll provide all my files incase the problem stretches across multiple errors.

So I have a module names 'Menu' that holds two classes, a completely private class 'MenuItem' and a owner of that class, 'Menu'.


Here is Menu.cpp

  #define _CRT_SECURE_NO_WARNINGS
#include "Menu.h"
#include "Utils.h"


namespace sdds {

	
	MenuItem::MenuItem() {
		m_item = nullptr;
	}

	MenuItem::MenuItem(const char* item) {
		if (item) {
			m_item = nullptr;

			//Create variables
			char buf;
			int i = 0;

			//Calculate length of char length
			do
			{
				buf = item[i];
				i++;
			} while (buf != '\0');

			//Allocate memory for m_item
			m_item = new char[i];

			//Copy item to m_item
			strncpy(m_item, item, i);
			m_item[i - 1] = '\0';
		} else {
			m_item = nullptr;
		}
	}

	std::ostream& MenuItem::display(std::ostream& out) const {
		//Check for validitity
		if (m_item) {
			//Output item and a new line
			out << m_item << std::endl;
		}

		return out;
	}

	std::ostream& operator<<(std::ostream& out, const MenuItem& mi) {
		mi.display(out);

		return out;
	}

	MenuItem::operator bool() const {
		//Return if object has a item
		return m_item ? 1 : 0;
	}

	MenuItem::~MenuItem() {
		//Delete m_item
		std::cout << "Deleting Menu Item";
		delete [] m_item;
	}


	//Menu Class
	//
	//

	void Menu::setTitle(const char* title) {

		if (!isEmpty()) {
			delete[] m_title;
			m_title = nullptr;

		}

		//Create variables
		char buf;
		int i = 0;

		//Calculate length of char length
		do
		{
			buf = title[i];
			i++;
		} while (buf != '\0');

		//Allocate memory for m_title
		m_title = new char[i];

		//Copy item to m_title
		strncpy(m_title, title, i);
		m_title[i - 1] = '\0';
	}

	Menu::Menu() {
		m_title = nullptr;
		m_menuItem[MAX_NO_OF_ITEMS] = { nullptr };
		m_menuCount = 0;
		m_indentation = 0;
	}

	Menu::Menu(const char* title, int indent) {
		if (title) {
			
			//Initialize m_title
			setTitle(title);

			//Initialize m_indentation
			if (indent >= 0) {
				m_indentation = indent;
			}

			//Intiialize other values
			m_menuItem[MAX_NO_OF_ITEMS] = { nullptr };
			m_menuCount = 0;
		}
		else {
			//Initialize Values
			m_title = nullptr;
			m_menuItem[MAX_NO_OF_ITEMS] = { nullptr };
			m_menuCount = 0;
			m_indentation = 0;
		}
	}

	Menu::~Menu() {
		for (int i = 0; i < m_menuCount; i++) {
			delete [] m_menuItem[i]->m_item;
			m_menuItem[i] = nullptr;
		}
		delete[] m_title;
		m_title = nullptr;
		m_menuCount = 0;
	}

	Menu& Menu::operator=(const Menu& menu) {
		if (!menu.isEmpty()) {
			//Shallow Copy
			m_indentation = menu.m_indentation;
			m_menuCount = 0;

			//Deep copy for resources
			setTitle(menu.m_title);

			for (int i = 0; i < menu.m_menuCount; i++) {
				add(menu.m_menuItem[i]->m_item);
			}
		}
		else {
			m_title = nullptr;
			m_menuItem[MAX_NO_OF_ITEMS] = { nullptr };
			m_menuCount = 0;
			m_indentation = 0;
		}

		return *this;
	}

	Menu& Menu::operator=(const char* title) {
		if (title) {
			setTitle(title);
		}
		else {
			m_title = nullptr;
		}

		return *this;
	}

	Menu& Menu::operator<<(const char* item) {
		if (item) {
			add(item);
		}
		else {
			m_title = nullptr;
			m_menuItem[MAX_NO_OF_ITEMS] = { nullptr };
			m_menuCount = 0;
		}
		
		return *this;
	}

	Menu::operator int()const{
		int rtn = run();
		return rtn;
	}

	Menu::operator bool() const {
		return !isEmpty();
	}

	bool Menu::isEmpty(void) const {
		return (m_title != nullptr) ? 0 : 1;
	}

	void Menu::add(const char* item) {
		if (item) {
			if (m_menuCount < MAX_NO_OF_ITEMS && !isEmpty()) {
				MenuItem* temp = new MenuItem(item);
				m_menuItem[m_menuCount] = temp;
				m_menuCount++;
			}
		}
		else {
			m_title = nullptr;
			m_menuItem[MAX_NO_OF_ITEMS] = { nullptr };
			m_menuCount = 0;
			m_indentation = 0;
		}
	}

	std::ostream& Menu::display(std::ostream& os) const {
		if (*this) {
			if (m_menuCount == 0) {
				//Print no items to display
				std::cout << "No Items to display!" << std::endl;
			}

			//Print Title
			std::cout.width(m_indentation * 4);
			std::cout << m_title << std::endl;
			//Print Options
			for (int i = 0; i < m_menuCount; i++) {
				std::cout.width(m_indentation * 4);
				std::cout << i + 1 << "- ";
				m_menuItem[i]->display(std::cout);
			}
			//Print dialouge
			std::cout.width(m_indentation * 4);
			std::cout << "> ";
		} else {
			std::cout << "Invalid Menu!" << std::endl;
		}

		return os;
	}


	int Menu::run(void) const {
		int o = 0;
		if (*this && m_menuCount > 0) {
			std::cin.clear();
			display();
			o = grabIntInRange(1, m_menuCount);
		}
		else {
			std::cout << "Invalid Menu!" << std::endl;
		}
		
		return o;

	}

}

Client code that produces memory leaks

Example 1

Menu invMenu("** To test Invalid Menu **");

Example 2

mainMenu = tempMenu;

Their are two memory leak locations, which add up to 25 errors via valgrind.

Example 1 (Only 1 error of this type)
==102889== 27 bytes in 1 blocks are definitely lost in loss record 20 of 20
==102889== at 0x4C2AC38: operator new[](unsigned long) (vg_replace_malloc.c:433)
==102889== by 0x4016B9: sdds::Menu::setTitle(char const*) (Menu.cpp:106)
==102889== by 0x40175A: sdds::Menu::Menu(char const*, int) (Menu.cpp:124)
==102889== by 0x400CDF: main (ms1_MenuTester.cpp:29)

Example 2 (Their are 24 of these errors stemming from different locations,
but all point to Menu.cpp:217)
==102889== 24 bytes in 3 blocks are definitely lost in loss record 19 of 20
==102889== at 0x4C2A593: operator new(unsigned long) (vg_replace_malloc.c:344)
==102889== by 0x401A4C: sdds::Menu::add(char const*) (Menu.cpp:217)
==102889== by 0x4018CB: sdds::Menu::operator=(sdds::Menu const&) (Menu.cpp:164)
==102889== by 0x400EE4: main (ms1_MenuTester.cpp:59)

First memory leak confuses me because I clearly check for data and delete accordingly, and the function in question gets called many times prior and no error occurs.

Second memory leak may be due to my lack of knowledge on the subject of pointers, pointer arrays, objects, and memory.

Any help is appreciated!

[helios]

1. Why are you allocating and copying char arrays manually instead of using std::string?
2. Why are you using a fixed length array of pointers instead of an std::vector of objects?
3. You do this in several places: m_menuItem[MAX_NO_OF_ITEMS] = { nullptr }; Are you sure it does what you think it does? It's not setting all elements of m_menuItem to nullptr, it's assigning nullptr to m_menuItem[MAX_NO_OF_ITEMS] (which is presumably one element past the end, thus overflowing the buffer).

[jonnin]

to clarify that a tiny bit:

something * m_menuItem[MAX_NO_OF_ITEMS] = { nullptr };
^^^ This one makes an array of pointers and sets them all nullptr. You probably saw this

m_menuItem[MAX_NO_OF_ITEMS] = { nullptr };
^^^ this one is a goof as described above. After seeing the above, maybe you crafted this one?

initialization is special in some ways, this is one of them.

[noahpereira20]

@helios
1. Its for an assignment, learning the fundamentals before learning the easy stuff
2. ^^^^^^
3.If I do not add the [MAX_NO_OF_ITEMS] it gives me an error, but thank you for letting me know.

[helios]

If I do not add the [MAX_NO_OF_ITEMS] it gives me an error, but thank you for letting me know.

Okay, but what did you replace those lines with? Maybe my tone wasn't clear enough, so allow me to be more explicit: those lines are incorrect. You're a) not initializing any of the pointers in m_menuItem, and b) writing to memory out of bounds. a) in particular is very likely the source of the crash, because your destructor has no way of knowing whether the pointers are non-null because they point to valid objects, or because you didn't initialize them.

EDIT: Oh, you said it was a leak, not a crash. It's still incorrect, though.

[jonnin]

you are going to need to pull this apart and exercise it a little bit at a time.

here is another oddity:

	Menu& Menu::operator<<(const char* item) {
		if (item) {
			add(item);
		}
		else {
			m_title = nullptr;  //did you just leak memory? how do you know?
			m_menuItem[MAX_NO_OF_ITEMS] = { nullptr };
			m_menuCount = 0;
		}
		
		return *this;
	}

[noahpereira20]

So, i rewrote the module based on the examples you guys pointed out, thanks for the replies, all works now!

[ne555]

> I'll provide all my files incase the problem stretches across multiple errors.
> Here is Menu.cpp
¿and the rest of the files?