Memory leak - i would recommend having better memory management in the program.

[McFrappe]

$ valgrind --leak-check=full ./pcalc
==28067== Memcheck, a memory error detector
==28067== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==28067== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==28067== Command: ./pcalc
==28067==
==28067== error calling PR_SET_PTRACER, vgdb might block
==28067==
==28067== HEAP SUMMARY:
==28067==     in use at exit: 104,380 bytes in 178 blocks
==28067==   total heap usage: 190 allocs, 12 frees, 117,482 bytes allocated
==28067==
==28067== LEAK SUMMARY:
==28067==    definitely lost: 0 bytes in 0 blocks
==28067==    indirectly lost: 0 bytes in 0 blocks
==28067==      possibly lost: 0 bytes in 0 blocks
==28067==    still reachable: 104,380 bytes in 178 blocks
==28067==         suppressed: 0 bytes in 0 blocks
==28067== Reachable blocks (those to which a pointer was found) are not shown.
==28067== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==28067==
==28067== For lists of detected and suppressed errors, rerun with: -s
==28067== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

The calculator might not be a big program but there is some memory leaks. As seen above, when starting through Valgrind, it shows, when the program has been stopped by CTRL-C that there is memory leakage. I see that in your numberstack.c you use malloc().

The amount of allocations at initialization is 190, whereas quitting instantly only causes 12 frees.

Perhaps i would recommend that you create a teardown function that tearsdown all your allocated structures.
As well that this teardown functionality occurs by some character that is used in the input pane in the program.

In short;

• Give the program an ability to type q or Q in input field (Quit the calculator)
• when this action is done, teardown internal structures in order to free memory

Maybe this is not your intention with the program but its a good practice to make as low of a memory leakage as possible.

[alt-romes]

This seems like a good idea.

Besides the input field quit command, we can also capture the Ctrl+C and call the teardown then.

[McFrappe]

This seems like a good idea.

Besides the input field quit command, we can also capture the Ctrl+C and call the teardown then.

That would be even greater! Do remember to include in your documentation, perhaps in your readme on how to use your calculator. 😄

[DevManu-de]

I was playing around with it a little bit and I am pretty sure most of those allocs come from ncurses WINDOW *

[alt-romes]

We still have to free them, I'm sure it's in the ncurses documentation somewhere :)

Having a teardown is just a pretty good idea overall, when "exit" or "q" or "ctrl+c" is executed

We also don't free the numberstack and other structures!

[McFrappe]

If you use endwin() for ncurses, it should already be handled (basically that command frees all associated thing allocated by ncurses)

[DevManu-de]

I wrote a very small program that basically does the same thing that pcalc but it also suffers from those memory leaks.
Also the endwin() that @McFrappe suggested doesnt really help.
If i remove the delwin(win); and WINDOW *win = the leak is even more worse.

I have to admit that I dont have much experience with ncurses and the documentation (https://invisible-island.net/ncurses/ncurses-intro.html#updating) doesnt really help either. Even the example has the same memory leaks.

Very small program I wrote

#include <stdio.h>
#include <ncurses.h>

int main()
{

    WINDOW *win = initscr();
    delwin(win);
    endwin();

    return 0;

}

valgrind --leak-check=full ./output/main

==461781== Memcheck, a memory error detector
==461781== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==461781== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==461781== Command: ./output/main
==461781==
==461781==
==461781== HEAP SUMMARY:
==461781==     in use at exit: 700,656 bytes in 142 blocks
==461781==   total heap usage: 211 allocs, 69 frees, 1,045,719 bytes allocated
==461781==
==461781== LEAK SUMMARY:
==461781==    definitely lost: 0 bytes in 0 blocks
==461781==    indirectly lost: 0 bytes in 0 blocks
==461781==      possibly lost: 0 bytes in 0 blocks
==461781==    still reachable: 700,656 bytes in 142 blocks
==461781==         suppressed: 0 bytes in 0 blocks
==461781== Reachable blocks (those to which a pointer was found) are not shown.
==461781== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==461781==
==461781== For lists of detected and suppressed errors, rerun with: -s

[McFrappe]

Weird...
Hm.

I might have to look into this tomorrow.
Silly question, did you try to free the pointer win?

[DevManu-de]

Yes I tried but this just adds one free to the total counter and nothing else.
So the new frees counter would be at 70.

[McFrappe]

Ah i think i have found the solution.
They discuss the following:

Perhaps you used a tool such as dmalloc or valgrind to check for memory leaks. It will normally report a lot of memory still in use. That is normal.

The ncurses configure script has an option, --disable-leaks, which you can use to continue the analysis. It tells ncurses to free memory if possible. However, most of the in-use memory is “permanent” (normally not freed).

Any implementation of curses must not free the memory associated with a screen, since (even after calling endwin()), it must be available for use in the next call to refresh(). There are also chunks of memory held for performance reasons. That makes it hard to analyze curses applications for memory leaks. To work around this, build a debugging version of the ncurses library which frees those chunks which it can, and provides the exit_curses() function to free the remainder on exit. The ncurses utility and test programs use this feature, e.g., via the ExitProgram() macro

So, basically i think its alright that ncurses "leaks" memory because it is if it would be used for the next instance when calling refresh().
But this can be disabled by a flag mentioned in there.

Though, as @alt-romes mentions, there are some teardown of structures to be done 😉

[DevManu-de]

Thanks for the information.

[DevManu-de]

I think we sould fix this really soon.
For small history sizes the leak is not that great but for larger sizes ~50 entries it gets to a real problem.

I will try to fix it as soon as possible but I also could need some help.

[alt-romes]

Hello!
I've merged the pull request related to this, and I've added handling for ctrl+c.

Can someone do a valgrind check for me please - so that I can close the issue?

Thank you,
~romes

[DevManu-de]

@alt-romes

I have experienced a problem but this problem only occurs after couple hundred inputs.
It seems like that numberstack causes the problem.

IMPORTANT: I already have a fix. #30

I looked into the code and think the problem is in this function. The realloc returns the pointer to s->elements which is a long long pointer and not a numberstack pointer or am I wrong here.

numberstack * resize_numberstack(numberstack* s) {

    s->max_size *= 2;
    return realloc(s->elements, s->max_size * sizeof(long long));
}

valgrind --leak-check=full -s ./pcalc

==503586== Memcheck, a memory error detector
==503586== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==503586== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==503586== Command: ./pcalc
==503586==
==503586==
==503586== HEAP SUMMARY:
==503586==     in use at exit: 1,389,787 bytes in 268 blocks
==503586==   total heap usage: 894 allocs, 627 frees, 1,532,804 bytes allocated
==503586==
==503586== 64 bytes in 1 blocks are definitely lost in loss record 11 of 45
==503586==    at 0x483CD7B: realloc (vg_replace_malloc.c:834)
==503586==    by 0x10A9E6: resize_numberstack (numberstack.c:21)
==503586==    by 0x10AA92: push_numberstack (numberstack.c:46)
==503586==    by 0x109A27: pushnumber (draw.c:147)
==503586==    by 0x10A21B: process_input (main.c:240)
==503586==    by 0x10A081: main (main.c:161)
==503586==
==503586== LEAK SUMMARY:
==503586==    definitely lost: 64 bytes in 1 blocks
==503586==    indirectly lost: 0 bytes in 0 blocks
==503586==      possibly lost: 0 bytes in 0 blocks
==503586==    still reachable: 1,389,723 bytes in 267 blocks
==503586==         suppressed: 0 bytes in 0 blocks
==503586== Reachable blocks (those to which a pointer was found) are not shown.
==503586== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==503586==
==503586== ERROR SUMMARY: 9 errors from 7 contexts (suppressed: 0 from 0)
==503586==
==503586== 1 errors in context 1 of 7:
==503586== Invalid free() / delete / delete[] / realloc()
==503586==    at 0x483B9AB: free (vg_replace_malloc.c:538)
==503586==    by 0x10AAF1: free_numberstack (numberstack.c:59)
==503586==    by 0x10A94A: exit_pcalc (main.c:418)
==503586==    by 0x491D69F: ??? (in /usr/lib/libc-2.32.so)
==503586==    by 0x49D0EBF: read (in /usr/lib/libc-2.32.so)
==503586==    by 0x4962541: _IO_file_underflow@@GLIBC_2.2.5 (in /usr/lib/libc-2.32.so)
==503586==    by 0x49637C5: _IO_default_uflow (in /usr/lib/libc-2.32.so)
==503586==    by 0x10A8A3: get_input (main.c:358)
==503586==    by 0x10A06E: main (main.c:159)
==503586==  Address 0x4c064d0 is 0 bytes inside a block of size 32 free'd
==503586==    at 0x483CD7B: realloc (vg_replace_malloc.c:834)
==503586==    by 0x10A9E6: resize_numberstack (numberstack.c:21)
==503586==    by 0x10AA92: push_numberstack (numberstack.c:46)
==503586==    by 0x109A27: pushnumber (draw.c:147)
==503586==    by 0x10A21B: process_input (main.c:240)
==503586==    by 0x10A081: main (main.c:161)
==503586==  Block was alloc'd at
==503586==    at 0x483A77F: malloc (vg_replace_malloc.c:307)
==503586==    by 0x10A983: create_numberstack (numberstack.c:12)
==503586==    by 0x109FF9: main (main.c:134)
==503586==
==503586== 1 errors in context 2 of 7:
==503586== Invalid write of size 8
==503586==    at 0x10AABA: push_numberstack (numberstack.c:48)
==503586==    by 0x109A27: pushnumber (draw.c:147)
==503586==    by 0x10A5DA: process_input (main.c:317)
==503586==    by 0x10A081: main (main.c:161)
==503586==  Address 0x4c064d0 is 0 bytes inside a block of size 32 free'd
==503586==    at 0x483CD7B: realloc (vg_replace_malloc.c:834)
==503586==    by 0x10A9E6: resize_numberstack (numberstack.c:21)
==503586==    by 0x10AA92: push_numberstack (numberstack.c:46)
==503586==    by 0x109A27: pushnumber (draw.c:147)
==503586==    by 0x10A21B: process_input (main.c:240)
==503586==    by 0x10A081: main (main.c:161)
==503586==  Block was alloc'd at
==503586==    at 0x483A77F: malloc (vg_replace_malloc.c:307)
==503586==    by 0x10A983: create_numberstack (numberstack.c:12)
==503586==    by 0x109FF9: main (main.c:134)
==503586==
==503586==
==503586== 1 errors in context 3 of 7:
==503586== Invalid write of size 8
==503586==    at 0x10AABA: push_numberstack (numberstack.c:48)
==503586==    by 0x10A6FC: process_input (main.c:344)
==503586==    by 0x10A081: main (main.c:161)
==503586==  Address 0x4c064e8 is 24 bytes inside a block of size 32 free'd
==503586==    at 0x483CD7B: realloc (vg_replace_malloc.c:834)
==503586==    by 0x10A9E6: resize_numberstack (numberstack.c:21)
==503586==    by 0x10AA92: push_numberstack (numberstack.c:46)
==503586==    by 0x109A27: pushnumber (draw.c:147)
==503586==    by 0x10A21B: process_input (main.c:240)
==503586==    by 0x10A081: main (main.c:161)
==503586==  Block was alloc'd at
==503586==    at 0x483A77F: malloc (vg_replace_malloc.c:307)
==503586==    by 0x10A983: create_numberstack (numberstack.c:12)
==503586==    by 0x109FF9: main (main.c:134)
==503586==
==503586==
==503586== 1 errors in context 4 of 7:
==503586== Invalid write of size 8
==503586==    at 0x10AABA: push_numberstack (numberstack.c:48)
==503586==    by 0x109A27: pushnumber (draw.c:147)
==503586==    by 0x10A21B: process_input (main.c:240)
==503586==    by 0x10A081: main (main.c:161)
==503586==  Address 0x4c064f0 is 0 bytes after a block of size 32 free'd
==503586==    at 0x483CD7B: realloc (vg_replace_malloc.c:834)
==503586==    by 0x10A9E6: resize_numberstack (numberstack.c:21)
==503586==    by 0x10AA92: push_numberstack (numberstack.c:46)
==503586==    by 0x109A27: pushnumber (draw.c:147)
==503586==    by 0x10A21B: process_input (main.c:240)
==503586==    by 0x10A081: main (main.c:161)
==503586==  Block was alloc'd at
==503586==    at 0x483A77F: malloc (vg_replace_malloc.c:307)
==503586==    by 0x10A983: create_numberstack (numberstack.c:12)
==503586==    by 0x109FF9: main (main.c:134)
==503586==
==503586==
==503586==
==503586== 2 errors in context 5 of 7:
==503586== Invalid read of size 8
==503586==    at 0x109773: draw (draw.c:99)
==503586==    by 0x10A097: main (main.c:164)
==503586==  Address 0x4c064e8 is 24 bytes inside a block of size 32 free'd
==503586==    at 0x483CD7B: realloc (vg_replace_malloc.c:834)
==503586==    by 0x10A9E6: resize_numberstack (numberstack.c:21)
==503586==    by 0x10AA92: push_numberstack (numberstack.c:46)
==503586==    by 0x109A27: pushnumber (draw.c:147)
==503586==    by 0x10A21B: process_input (main.c:240)
==503586==    by 0x10A081: main (main.c:161)
==503586==  Block was alloc'd at
==503586==    at 0x483A77F: malloc (vg_replace_malloc.c:307)
==503586==    by 0x10A983: create_numberstack (numberstack.c:12)
==503586==    by 0x109FF9: main (main.c:134)
==503586==
==503586==
==503586== 2 errors in context 6 of 7:
==503586== Invalid read of size 8
==503586==    at 0x10A6AA: process_input (main.c:340)
==503586==    by 0x10A081: main (main.c:161)
==503586==  Address 0x4c064f0 is 0 bytes after a block of size 32 free'd
==503586==    at 0x483CD7B: realloc (vg_replace_malloc.c:834)
==503586==    by 0x10A9E6: resize_numberstack (numberstack.c:21)
==503586==    by 0x10AA92: push_numberstack (numberstack.c:46)
==503586==    by 0x109A27: pushnumber (draw.c:147)
==503586==    by 0x10A21B: process_input (main.c:240)
==503586==    by 0x10A081: main (main.c:161)
==503586==  Block was alloc'd at
==503586==    at 0x483A77F: malloc (vg_replace_malloc.c:307)
==503586==    by 0x10A983: create_numberstack (numberstack.c:12)
==503586==    by 0x109FF9: main (main.c:134)
==503586==
==503586== ERROR SUMMARY: 9 errors from 7 contexts (suppressed: 0 from 0)

DevManu-de

[alt-romes]

The problem is not due to it not being a numberstack - the realloc is to resize the list of elements (of type long long)

However, I just noticed the realloc is a false return as you just said in your PR.

Thank you
~romes