Search code examples
c++stackvalgrind

Memory leak in C++ (Valgrind)


I implement the stack with a minimum. In this program, I get an error from valgrind. Something is wrong with the push() and main() functions. When I add delete st; to the push() function, I get even more errors. I check it through valgrind ./a.out. Sorry for the long code. I also wrote the rest of the functions for stack. But there is no error in them, I left those in the code where there may be an error.

#include <cstring>
#include <iostream>

struct Stack {
  int data;
  int min;
  Stack* next;
};

void Push(Stack** top, int n) {
  Stack* st = new Stack();
  st->data = n;
  if (*top == NULL) {
    *top = st;
    (**top).min = n;
  } else {
    st->min = ((n <= (**top).min) ? n : (**top).min);
    st->next = *top;
    *top = st;
  }
  std::cout << "ok" << std::endl;
}

void Pop(Stack** top) {
  if (*top != NULL) {
    std::cout << (**top).data << std::endl;
    *top = (*top)->next;
  } else {
    std::cout << "error" << std::endl;
  }
}

int main() {
  Stack* top = nullptr;
  int m;
  std::cin >> m;
  std::string str;
  for (int i = 0; i < m; ++i) {
    std::cin >> str;
    if (str == "push") {
      int value;
      std::cin >> value;
      Push(&top, value);
    }
    if (str == "pop") {
      Pop(&top);
    }
  }
  delete top;
}

Solution

  • When you just delete top, you destruct it (in your case it's nothing, but you can distract yourself for reading about destructors if interested) and free the dynamic memory allocated for top. However, you actually want to also delete top->next, top->next->next (if present) etc. A hotfix:

    while (top) { // same as "while (top != nullptr) {"
        Stack* next = top->next; // we can't use `top` after we `delete` it, save `next` beforehand
        delete top;
        top = next;
    }
    

    Now, about more general things. The course teaches you some really old C++ (almost just plain C; even C here is bad though). At the very least, your whole Push() can be replaced (thanks to lvalue references (Type&), std::min and aggregate initialization) with:

    void push(Stack*& top, int n) {
        top = new Stack{n, std::min(n, top ? top->min : n), top};
        std::cout << "ok\n";
    }
    

    I'm new to C++ programming. I used to write in Python

    Good job. Sadly, such teaching shows C++ as something too old and horrifying.

    Edit

    here's a new in Push, so there should most likely be a delete in Pop

    That's right (thanks to @molbdnilo). You should delete popped elements instead of just leaking them.