Need Help Solving a Memory Leak

Question

I am writing an inventory management system using dynamic arrays using objects. The strategy for this is NOT to use vectors, but allocate a dynamic array that 1 increment by 1 every time the client needs to add to the inventory. I am getting a "segmentation fault" error so I believe it is a memory leak.

I have tried rewriting it to match addresses but no luck. I think the buildItem function produces a temporary object, which is destroyed when the function ends. I don't know how to fix this though.

#include <iomanip>
#include <iostream>
#include <string>
using namespace std;

class InventoryObject {
private:
  int itemNumber;
  string description;
  int qty;
  float price;

public:
  int getItemNum() { return itemNumber; }
  string getDescription() { return description; }
  int getQty() { return qty; }
  float getPrice() { return price; }

  void storeInfo(int p, string d, int q, float pr);
  void showValues(InventoryObject &item);
};

// Function Implementation
void InventoryObject::storeInfo(int p, string d, int q, float pr) {
  itemNumber = p;
  description = d;
  qty = q;
  price = pr;
}

void InventoryObject::showValues(InventoryObject &item) {
  cout << fixed << showpoint << setprecision(2) << endl;
  cout << "Part Number  : " << item.getItemNum() << endl;
  cout << "Description  : " << item.getDescription() << endl;
  cout << "Quantity:    : " << item.getQty() << endl;
  cout << "Price        : " << item.getPrice() << endl << endl;
}

// Function Prototypes for Client Program
InventoryObject buildItem();
void drawMenu();
void showValues(InventoryObject &);
void printInventory(int size);

int main() {
  int size = 1;
  int choice;
  bool quit = false;
  InventoryObject part;
  InventoryObject *iArray = new InventoryObject[size];
  drawMenu();
  cin >> choice;
  while (quit == false) {
    if (choice == 1) {
      InventoryObject item;
      item = buildItem();
      iArray[size] = item;
    }
    if (choice == 2) {
      iArray[size].showValues(iArray[size]);
    }
    if (choice == 3) {
      quit = true;
    }
  }

  return 0;
}

// This function accepts the data from the client and creates a new
// InventoryObject object. The object is then supposed to be added to the
// dynamic array.
InventoryObject buildItem() {
  InventoryObject *tempObject = new InventoryObject;
  int itemNum;
  string description;
  int qty;
  float price;

  cout << "Enter data for the item you want to enter:\n\n";
  cout << "Item Number: \n";
  cin >> itemNum;
  cout << "Description: \n";
  cin.get();
  getline(cin, description);
  cout << "Quantity: \n";
  cin >> qty;
  cout << "Unit price: \n";
  cin >> price;

  tempObject->storeInfo(itemNum, description, qty, price);
  return *tempObject;
}

void drawMenu() {
  cout << "1. Add Inventory\n";
  cout << "2. Display Inventory\n";
  cout << "3. Quit Program\n";
}

I expect the object to be created, and put into the dynamic array. Then redraw the menu and interact with the client from there.

Answer 1

The main problem is that you are writing past the area of memory you allocated for your array (iArray).

Specifically, this line of code:

iArray[size] = item;

Should actually be:

iArray[size - 1] = item;

---

The above does not cause memory leak, but something else you do in your program does:

Your function buildItem does this when it returns the value of a pointer without first deleting the pointer.

To fix this, change

InventoryObject *tempObject = new InventoryObject;

to

InventoryObject tempObject;

And finally, remember to delete iArray before return 0; in main

delete[] iArray

Answer 2

1. Build your program with debug symbol (-g flag)
2. Run limit-c unlimited
3. Run your program until it crashes
4. Run gdb core where is the name of your program
5. In the gdb climate type bt.

You should now have a back trace of where your program is crashing

Answer 3

Daniel, here my observations:

1. The function buildItem must return a pointer of InventoryObject i mean: InventoryObject*. It is bease you are returning the pointer of the memory that was reserved at the moment of create it (InventoryObject *tempObject = new InventoryObject;), like this:

InventoryObject* buildItem() {
  InventoryObject *tempObject = new InventoryObject;

  //...

  tempObject->storeInfo(itemNum, description, qty, price);
  return *tempObject;
}

keep in mind that your main array (InventoryObject *iArray) contains an array of pointer.

2. The main problem that i see. In the main you are using an if block where there is a local variable called item (InventoryObject item). Let me explain something: Each instance of a class, like you InventoryObject item will be destroyed, release its memory, if its context is over.

So, in this case, the problem is that you was casting the pointer of at the moment of returning from the function buildItem (return *tempObject) and store its value into a local variable InventoryObject item which shall be release when the thread leaves the if, the context of the local variable.

Change your local variable InventoryObject item to handle a pointer: InventoryObject* item

    if (choice == 1) {
      InventoryObject* item;
      item = buildItem();
      iArray[size] = item;
    }

I recommend you to read the chapters 7. Pointers, Arrays, and References and 17 Construction, Cleanup, Copy, and Move from the book The C++ Programming Language of Bjarne Stroustrup.

Answer 4

I have tried rewriting it to match addresses but no luck. I think the buildItem function produces a temporary object, which is destroyed when the function ends.

Your intuition is half true. There is something wrong with this function, but it has nothing to do with temporaries. It's more about you allocating an object you never destroy.

The problem is that you are trying to do something similar to what I was trying to do in this question, but not quite. Even if you were doing what I was doing, it still wouldn't be a good idea for reasons that lots of people already explained better than I could.

The point is is that the base of the issue is that is if you use new to allocate something, you must use delete to deallocate it, which you don't do here.

InventoryObject buildItem() {
  InventoryObject *tempObject = new InventoryObject;
  //...
  return *tempObject;
}

You simply return the object by value without deleting it first. You aren't even returning it by reference which would be better (yet still probably bad: again, see my question). You are allocating an object and never deleting it. That's a leak.

You have two solutions: either return the pointer directly (if you really need a pointer) like this:

InventoryObject* buildItem() {
  InventoryObject *tempObject = new InventoryObject;
  //...
  return tempObject;
}

Or, you can simply just return the object by value like this:

InventoryObject buildItem() {
  InventoryObject tempObject;
  //...
  return tempObject;
}

Given the code sample you've shown, this is my recommendation of the two.

---

Note: I should mention that if for some reason you do need to return a pointer and have some factory method pattern with some sort of polymorphism in play, you also have the option of smart pointers too. That is what I ended up doing based on other people's suggestions from the other thread. I can personally recommend it. Basically, your code code would become something like this instead:

std::unique_ptr<InventoryObject> buildItem() {
  std::unique_ptr<InventoryObject> tempObject = new InventoryObject; // C++11 way, I think
                  // C++14 and above has a function called make_unique you could use instead.
  //...
  return tempObject;
}

However, this is only useful if you have some sort of polymorphism going on as I did. Given the code sample you have posted so far, you really only need to return by value in your situation. I just thought I'd mention this other option too, just in case.

---

P. (P.) S. This answer is only about your suspected memory leak. That said, there are other helpful answers here that you should heed. Problems described in other answers could contribute to your problem and could really trip you up down the road (actually, could still trip you up now too) if you don't fix them, so I highly recommend taking their advice too.