Bug 65434 - Memory leak in pool constructor
Summary: Memory leak in pool constructor
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 5.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-16 11:03 UTC by Dmitry Shachnev
Modified: 2019-06-15 00:32 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
Provide optional "static" memory-allocation in eh_alloc.cc (on demand) (1.36 KB, patch)
2016-12-13 08:24 UTC, Markus Eisenmann
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Shachnev 2015-03-16 11:03:19 UTC
Constructor of `pool' class in eh_alloc.c has the following code:

pool::pool()
  {
    // Allocate the arena - we could add a GLIBCXX_EH_ARENA_SIZE environment
    // to make this tunable.
    arena_size = (EMERGENCY_OBJ_SIZE * EMERGENCY_OBJ_COUNT
                  + EMERGENCY_OBJ_COUNT * sizeof (__cxa_dependent_exception));
    arena = (char *)malloc (arena_size);
    ....
  }

The memory allocated by `malloc (arena_size)' is never freed, because that class does not have a destructor. This results in a memory leak. Valgrind reports:

18,944 bytes in 1 blocks are still reachable in loss record 1 of 1
   at 0x40291CC: malloc (vg_replace_malloc.c:296)
   by 0x40D630A: pool (eh_alloc.cc:117)
   by 0x40D630A: __static_initialization_and_destruction_0 (eh_alloc.cc:244)
   by 0x40D630A: _GLOBAL__sub_I_eh_alloc.cc (eh_alloc.cc:307)
   by 0x400E86D: call_init.part.0 (dl-init.c:78)
   by 0x400E963: call_init (dl-init.c:36)
   by 0x400E963: _dl_init (dl-init.c:126)
   by 0x4000D3E: ??? (in /lib/i386-linux-gnu/ld-2.19.so)

This happens with the current gcc-5 snapshot, but did not happen with 4.9.

It was broken in revision 219988 (PR libstdc++/64535).
Comment 1 Jonathan Wakely 2015-03-16 15:09:47 UTC
(In reply to Dmitry Shachnev from comment #0)
> 18,944 bytes in 1 blocks are still reachable in loss record 1 of 1

"still reachable" is not a leak, the memory is still in use by the runtime.

This is intentional.
Comment 2 Dmitry Shachnev 2015-03-16 15:33:43 UTC
Will anything bad happen if that memory is freed in the destructor?

For me, the issue is mostly aesthetic — I got used to not seeing any Valgrind warnings in my programs :)
Comment 3 Jonathan Wakely 2015-03-16 15:59:57 UTC
(In reply to Dmitry Shachnev from comment #2)
> Will anything bad happen if that memory is freed in the destructor?

Yes, because other destructors could run later, and could (potentially) need to use the pool after it had been destroyed. That would be undefined behaviour.

> For me, the issue is mostly aesthetic — I got used to not seeing any
> Valgrind warnings in my programs :)

Then add a valgrind suppression, or ask upstream valgrind to add it.
Comment 4 Emil Styrke 2016-01-26 13:51:34 UTC
I just discovered that this will become a real leak if dlopen/dlclose:ing a library that has been statically linked to libstdc++.  Each time the library is opened a new pool object is created, and after dlclose it will not have any references left. Valgrind shows this (after looping the dlopen/dlsym/dlclose part a few times):

==3679== 3,707,904 bytes in 51 blocks are definitely lost in loss record 106 of 106
==3679==    at 0x4C2BBCF: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3679==    by 0x77335EF: _GLOBAL__sub_I_eh_alloc.cc (in [...]/libmock_di_plugin.so)
==3679==    by 0x40105B9: call_init.part.0 (dl-init.c:72)
==3679==    by 0x40106CA: call_init (dl-init.c:30)
==3679==    by 0x40106CA: _dl_init (dl-init.c:120)
==3679==    by 0x4015586: dl_open_worker (dl-open.c:579)
==3679==    by 0x4010463: _dl_catch_error (dl-error.c:187)
==3679==    by 0x40149A2: _dl_open (dl-open.c:663)
==3679==    by 0x6B94FC8: dlopen_doit (dlopen.c:66)
==3679==    by 0x4010463: _dl_catch_error (dl-error.c:187)
==3679==    by 0x6B9562C: _dlerror_run (dlerror.c:163)
==3679==    by 0x6B95060: dlopen@@GLIBC_2.2.5 (dlopen.c:87)
==3679==    by 0x4F303E6: load_library (dipluginhandlerimpl.cpp:60)

I'm not sure if anything could be done about this, but it might be prudent to add a warning to the documentation for -lstatic-libstdc++ explaining this.
Comment 5 Patrick J. LoPresti 2016-04-15 17:55:27 UTC
Seems pretty sloppy not to free what you allocate, and then demand all leak checking tools forever work around the sloppiness... Even if you are the runtime.

Couldn't you fix this by using the init_priority attribute on emergency_pool?

Alternatively, do like any application would and use a Meyers singleton instead of a global variable? Like so:

namespace {
  pool &emergency_pool()
    {
      static pool emp;
      return emp;
    }
}

Since destructors always run in the opposite order of construction, you would just need to make sure you obtain the first reference early enough.
Comment 6 Markus Eisenmann 2016-12-13 08:24:19 UTC
Created attachment 40318 [details]
Provide optional "static" memory-allocation in eh_alloc.cc (on demand)

This patch provides a minor extension to allocate the emergency-buffer from BSS, if the needed size is bellow a limit (macro LIMIT_EMERGENCY_BSS_MEM, e.g. 4096 Byte).
Comment 7 Markus Eisenmann 2016-12-13 09:00:01 UTC
Hi!

My motivation to use/implement this patch (comment #6) is to prevent using malloc to allocate the needed emergency-buffer region, if the needed overall size is bellow a (configurable) limit; e.g., embedded targets without threading-support.

It does not change the behaviour of the sub-allocator (pool), but will allow to apply/use a static memory-region (from BSS), which will solve this kind of memory-leak (if the second - I.e, static - memory-provider template will choosen).

Best regards from Salzburg,
Markus
Comment 8 Jonathan Wakely 2016-12-13 11:19:52 UTC
Please send patches to the libstdc++@ and gcc-patches@ mailing lists, rather than attaching them to closed bugs.

https://gcc.gnu.org/onlinedocs/libstdc++/manual/appendix_contributing.html
Comment 9 Jonathan Wakely 2016-12-13 13:21:01 UTC
(Although I have a simpler patch that does something similar, as well as allowing the arena size to be controlled form the environment).
Comment 10 Jonathan Wakely 2016-12-13 13:55:27 UTC
See https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01158.html