Opened 10 years ago

Closed 10 years ago

#8079 closed Bugs (fixed)

Chrono memory leak

Reported by: dm413-boost@… Owned by: viboes
Milestone: Boost 1.54.0 Component: chrono
Version: Boost 1.53.0 Severity: Problem
Keywords: Cc:

Description

The constructor for duration_units_default_initializer_t (in chrono/io/duration_units.hpp) allocates two string_type arrays from the heap but never deletes them. The Visual C++ MFC debugger reports this as a memory leak (and I expect most other leak detectors will as well).

These variables are class static and only one instance of this class (per CharT) is allocated anyway, so this is not a "serious" memory leak, but any memory leak reported by a leak detector is worth fixing to avoid masking other more serious memory leaks.

I suggest adding a destructor similar to this:

        ~duration_units_default_initializer_t()
          {
            if (duration_units_default_holder<CharT>::initialized_)
            {
              delete[] duration_units_default_holder<CharT>::n_d_valid_units_;
              duration_units_default_holder<CharT>::n_d_valid_units_ = 0;
              delete[] duration_units_default_holder<CharT>::valid_units_;
              duration_units_default_holder<CharT>::valid_units_ = 0;
              duration_units_default_holder<CharT>::initialized_ = false;
            }
    	  }

Further suggestions for this code:

  1. Make the class static member variables n_d_valid_units_ and valid_units_ into non-static member variables. Only one object of this type is created, so making them them regular member variables makes things clearer and simplifies the code.
  1. Do away with the initialized_ member variable entirely. If you make the other member variables non-static, there is no need for this variable.
  1. If I am wrong and it is possible to create more than one instance of duration_units_default_initializer_t<CharT>, then you will need to make the destructor more intelligent and use smart pointers or a reference count to know when to destroy the static member variables.

Change History (4)

comment:1 by viboes, 10 years ago

You are not wrong, there is only one instance.

Changing all the static members to non-static implies to store a pointer to such a single object. Maybe this could be more elegant, but this adds an indirection. I would not be against a patch, but currently I have no time for such a refactoring.

Have you checked your suggestion of deleting the allocated memory on the destructor in a multi DLL environment?

comment:2 by dm413-boost@…, 10 years ago

I have not tested in a multi-DLL environment, I have been working with statically linked boost. Off-hand I don't see a problem -- wouldn't each DLL have it's own copy of the static variables?

comment:3 by viboes, 10 years ago

Milestone: To Be DeterminedBoost 1.54.0
Status: newassigned

Your are right. Committed in trunk [83510].

comment:4 by viboes, 10 years ago

Resolution: fixed
Status: assignedclosed

Merged revision [83624].

Note: See TracTickets for help on using tickets.