devosoft / Empirical

A library of tools for scientific software development, with emphasis on also being able to build web interfaces using Emscripten.
Other
86 stars 38 forks source link

Fix memory leak in emp::DataFile #467

Open FergusonAJ opened 2 years ago

FergusonAJ commented 2 years ago

Bug description One of the constructors in emp::DataFile doesn't take an std::ostream, instead it takes a filename string. It then uses that string to create a new std::ostream pointer. However, this pointer (os) is never deleted. Since it's not an Empirical pointer, this is not detected in debug mode.

Relevant code (line 52 at time of writing):

    DataFile(const std::string & in_filename,
             const std::string & b="", const std::string & s=",", const std::string & e="\n")
      : filename(in_filename), os(
      #ifdef __EMSCRIPTEN__
      new emp::NullStream()
      #else
      new std::ofstream(in_filename)
      #endif
      ), funs(), pre_funs(), keys(), descs()
      , timing_fun([](size_t){return true;})
      , line_begin(b), line_spacer(s), line_end(e) { ; }

To Reproduce The DataFile example (examples/data/DataFile.cpp) has this issue without any modifications. Here's the valgrind command I used to show the issue:

valgrind --leak-check=full --show-reachable=yes --leak-resolution=high --num-callers=100 --trace-children=yes ./DataFile 

Expected behavior Removing the memory leak is the main priority. Switching the os pointer to be an emp::Ptr would improve consistency with rest of codebase. Destructor and copy/move constructors will likely need to be updated to accommodate issue (rule of five).

Toolchain (please complete the following information):