vigna / ne

ne, the nice editor
http://ne.di.unimi.it/
GNU General Public License v3.0
473 stars 33 forks source link

Memory leak after opening file from file browser #116

Closed correctmost closed 9 months ago

correctmost commented 9 months ago

Steps to reproduce:

  1. Run ne
  2. Ctrl + o
  3. Open a file (I opened CHANGES from the ne repo)
  4. Alt + q

Leak report:

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x77a3946e1359 in __interceptor_malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x55d48a4fae89 in str_dup /home/s/code/ne/src/support.c:363
    #2 0x55d48a4fb966 in relative_file_path /home/s/code/ne/src/support.c:141
    #3 0x55d48a4e949f in request_files /home/s/code/ne/src/request.c:1145
    #4 0x55d48a4e9a38 in request_file /home/s/code/ne/src/request.c:1181
    #5 0x55d48a3b23ee in do_action /home/s/code/ne/src/actions.c:1038
    #6 0x55d48a410472 in execute_command_line /home/s/code/ne/src/command.c:340
    #7 0x55d48a47fd77 in main /home/s/code/ne/src/ne.c:545
    #8 0x77a393c43ccf  (/usr/lib/libc.so.6+0x25ccf)
    #9 0x77a393c43d89 in __libc_start_main (/usr/lib/libc.so.6+0x25d89)
    #10 0x55d48a399934 in _start (/usr/local/bin/ne+0x2f0934)

Version: c2c58b96cd289fb003fb51d88f7af7ede0f410cc

utoddl commented 9 months ago

You're not wrong!

On the other hand, once all the documents the user wants to save have been saved and we're going to quit, there's really no point in de-allocating outstanding allocations as they'll all be recovered on exit. We could add extra code to meticulously traverse all the dynamic structures and free everything before exiting, but there would be no benefit. The only effects would be that the executable would be larger and exiting the program would be slower.

At least, that's the assumption we've been working under until now. If there's a counter case to be made, I would certainly like to know about it. Otherwise, I'm disinclined to add code to address this particular issue.

Even so, I'll leave this issue open for now to allow space for other opinions.

Last but not least, thanks for your interest in the project. We really do appreciate the time and effort people put in, even if we sometimes choose not to act on a particular issue.

correctmost commented 9 months ago

Thanks for taking a look. I think the primary benefit of cleaning up structures before exiting is that it can make it easier to detect new leaks that are introduced.

As you mentioned, though, it does require extra code, extra processing, and extra testing. If the general strategy is to let exit handle some of the deallocations, then it probably doesn't make sense to patch just this issue :).

utoddl commented 9 months ago

That's a legitimate point. We do have other code that is conditionally included, for example only for the test suite, but that isn't part of a "regular" build. Perhaps it does make sense to conditionally include complete clean-up code so that, as you say, detecting new leaks would be easier. Must give this some thought.

vigna commented 9 months ago

In the meantime, I think we can consider this issue closed.