valgrind reports 'potential memory leaks' from gflags

[schuhschuh]

Original issue 40 created by schuhschuh on 2010-12-13T21:23:29.000Z:

What steps will reproduce the problem?

1. compile a program using gflags
2. run it with 'valgrind --leak_check=full'
3. look at all the memory leak reports from gflags

What is the expected output? What do you see instead?

expected: clean memory leak report
instead: saw lots of potential memory leaks

These are not really memory leaks, but the presence of these leaks makes it harder to see real memory leaks. Note that 'potential' memory leaks are sometimes quite dangerous. For example, when working with Apache HTTPD, a large amount of memory allocation is using a pooled allocator. You can leak memory into pools, and valgrind will report that as a 'potential leak'.

The gflags leak reports are not really problems like Apache pools, but the presence of them can obscure real problems.

What version of the product are you using? On what operating system?

http://google-gflags.googlecode.com/svn/tags/gflags-1.3/src (head)
OS: Linux Ubtuntu 10.04.

Please provide any additional information below.

[schuhschuh]

Comment #1 originally posted by schuhschuh on 2010-12-13T22:30:29.000Z:

I'm ok with adding something like this if it doesn't interfere with normal usage (that is, most people can ignore it, and it doesn't complexify understanding of the code). Should be doable for the next release.

[schuhschuh]

Comment #2 originally posted by schuhschuh on 2011-01-25T00:33:51.000Z:

A function that frees memory was added in gflags 1.5, just released.

[schuhschuh]

Comment #3 originally posted by schuhschuh on 2011-01-25T00:54:49.000Z:

Quick question on this. I tried to update my open-source dependencies to
bring in version 1.5 but I got a 'url is unreachable error' from 'gclient':

Error: This url is unreachable:
http://google-gflags.googlecode.com/svn/tags/gflags-1.5/src@head

I went over to http://code.google.com/p/google-gflags/source/browse/ and
looked at the tags. I don't see a gflags-1.5 yet. Is that coming?

Thanks!

Directories

• svn http://code.google.com/p/google-gflags/source/browse/
  • brancheshttp://code.google.com/p/google-gflags/source/browse/branches
    • tags http://code.google.com/p/google-gflags/source/browse/tags
    • gflags-0.1http://code.google.com/p/google-gflags/source/browse/#
    • gflags-0.2http://code.google.com/p/google-gflags/source/browse/#
    • gflags-0.3http://code.google.com/p/google-gflags/source/browse/#
    • gflags-0.4http://code.google.com/p/google-gflags/source/browse/#
    • gflags-0.5http://code.google.com/p/google-gflags/source/browse/#
    • gflags-0.6http://code.google.com/p/google-gflags/source/browse/#
    • gflags-0.7http://code.google.com/p/google-gflags/source/browse/#
    • gflags-0.8http://code.google.com/p/google-gflags/source/browse/#
    • gflags-0.9http://code.google.com/p/google-gflags/source/browse/#
    • gflags-1.0rc1http://code.google.com/p/google-gflags/source/browse/#
    • gflags-1.0rc2http://code.google.com/p/google-gflags/source/browse/#
    • gflags-1.1http://code.google.com/p/google-gflags/source/browse/#
    • gflags-1.2http://code.google.com/p/google-gflags/source/browse/#
    • gflags-1.3http://code.google.com/p/google-gflags/source/browse/#
    • gflags-1.4http://code.google.com/p/google-gflags/source/browse/#
  • trunk http://code.google.com/p/google-gflags/source/browse/trunk

[schuhschuh]

Comment #4 originally posted by schuhschuh on 2011-01-25T01:32:50.000Z:

My mistake -- it looks like my svn tag command failed. I just ran it again, so everything should be fixed now. Thanks for pointing this out!

[schuhschuh]

Comment #5 originally posted by schuhschuh on 2012-09-13T14:08:32.000Z:

It looks like this is still leaking std::strings for DEFINE_string constants.

Is there any way to clean those up too?

I can add valgrind suppression for that, but it still gives me errors if I when I change the default flag value (as shown here http://google-gflags.googlecode.com/svn/trunk/doc/gflags.html#default), which are tedious to have to go through and individually suppress.

[schuhschuh]

Comment #6 originally posted by schuhschuh on 2012-09-13T14:12:04.000Z:

Yes. You need to clean up the constants on exit, which you can do by calling ShutDownCommandLineFlags().

[schuhschuh]

Comment #7 originally posted by schuhschuh on 2012-09-14T09:27:11.000Z:

I'm already calling this on shutdown, but I still get valgrind (3.6.1) complaining. That said, I've committed the cardinal sin of whinging on a bug but having a slightly old version of both valgrind and gflags. I'll update both and try again, and get back to you with a better report. Thanks.

[schuhschuh]

Comment #8 originally posted by schuhschuh on 2012-09-18T10:58:40.000Z:

OK, with the latest valgrind and gflags this is still an issue, even with the most basic possible program:

#include <gflags/gflags.h>

DEFINE_bool(foo, false, "Test flag");
DEFINE_string(bar, "bar", "bar flag");

int main(int argc, char** argv)
{
google::ShutDownCommandLineFlags();
return 0;
}

If I remove the DEFINE_string() then I get no warnings.

It seems that the std::string that DEFINE_string is allocating is leaking (I guess in line 530 of gflags.h, where it's allocated, or 551 where it's kept but never freed, depending how you look at it):

==18700== HEAP SUMMARY:
==18700== in use at exit: 28 bytes in 1 blocks
==18700== total heap usage: 82 allocs, 81 frees, 3,116 bytes allocated
==18700==
==18700== 28 bytes in 1 blocks are possibly lost in loss record 1 of 1
==18700== at 0x4A0867F: operator new(unsigned long) (vg_replace_malloc.c:298)
==18700== by 0x3A5E89B860: std::string::Rep::S_create(unsigned long, unsigned long, std::allocator const&) (in /usr/lib64/libstdc++.so.6.0.8)
==18700== by 0x3A5E89C364: ??? (in /usr/lib64/libstdc++.so.6.0.8)
==18700== by 0x3A5E89C511: std::basic_string<char, std::char_traits, std::allocator >::basic_string(char const, std::allocator const&) (in /usr/lib64/libstdc++.so.6.0.8)
==18700== by 0x400AFF: fLS::dont_pass0toDEFINE_string(char, char const*) (in /home/[...]/foo)
==18700== by 0x4009C7: __static_initialization_and_destruction_0(int, int) (in /home/[...]/foo)
==18700== by 0x400A85: global constructors keyed to fLB::FLAGS_foo (in /home/[...]/foo)
==18700== by 0x400C25: ??? (in /home/[...]/foo)
==18700== by 0x4007BA: ??? (in /home/[...]/foo)
==18700== by 0x3A5EAEF0FF: ???
==18700== by 0x400BA6: __libc_csu_init (in /home/[...]/foo)
==18700== by 0x3A5CC1D92D: (below main) (in /lib64/libc-2.5.so)

[schuhschuh]

Comment #9 originally posted by schuhschuh on 2012-09-18T13:27:06.000Z:

OK, thanks for the report and nice example !

There are two uses of placement-new, one at line 530 and the other at 556 of gflags.h. In both cases, the strings are passed on to FlagValue(), which never takes over ownership of the memory and thus will not free it upon ShutDownCommandLineFlags(). So I am not clear yet why only the first would cause a valgrind warning...

I have compiled your example and run it through valgrind 3.6.1. I get the same, so I reopen this bug and will do some debugging and see if we can get rid of this last valgrind warning.

[schuhschuh]

Comment #10 originally posted by schuhschuh on 2012-09-19T07:51:12.000Z:

OK, I figured it out. What is missing is the explicit destructor call for the string objects that were created via placement-new. See also http://www.stroustrup.com/bs_faq2.html#placement-delete . Adding these manually to the example made the valgrind warning disappear.

Now that I know what is missing and how to close this leak, I just need to figure how best to patch the code...

[schuhschuh]

Comment #11 originally posted by schuhschuh on 2012-11-20T11:25:49.000Z:

I added the placement delete template function from http://www.stroustrup.com/bs_faq2.html#placement-delete to gflags.cc and modified the destructor of FlagValue to call it for string objects created via placement new. This, however, given your provided example code, did not help to resolve this issue. Only when I add a call to the placement delete for both string objects created by DEFINE_string for the current and default value after ShutDownCommandLineFlags() in main(), the valgrind warnings disappear.

It seems to somewhat matter in which module the destructor of the object is called. If called in the main module, i.e., the one containing the corresponding DEFINE_string, it seems to work. If called within gflags.cc, however not... (linking statically to gflags library).

Also, given the issue at hand, it is surprising that their are no warnings for those string flags defined in gflags_reporting.cc, even without explicit call of the destructor of placement new-created string objects.

Maybe somebody can enlighten me ? I will keep looking for a solution, but feel like I'm missing something important yet.

[schuhschuh]

Comment #12 originally posted by schuhschuh on 2013-07-31T11:37:29.000Z:

Is this issue still open?

I experienced that valgrind warnings only appear, when a default value for the string is given:

DEFINE_string(foo, "c", "foo");//valgrind warning reports "..possibly lost.."
DEFINE_string(foo, "", "foo");//no valgrind report

Hopefully this helps to get closer to the fix.

[schuhschuh]

Comment #13 originally posted by schuhschuh on 2013-08-20T09:17:19.000Z:

Bump.

Any news on this?

[schuhschuh]

Comment #14 originally posted by schuhschuh on 2013-08-20T09:21:55.000Z:

Hello, I replied by mail a few weeks ago wrongly assuming the reply would get attached here. It wasn't. I decided to just dump below my original message, dated 31 Jul.

• - - - - (in reply to message 31 Jul) - - - - -
  Yes and no.
  Looks like both me and Andreas forgot to add a note about this.

There's a branch (bugfix-40-memory-leaks) which fixed this behaviour.
Howerer, due to compatibility concerns, it has not been merged with master, default version.

I've implemented the fix myself but Andreas took it one step further by seeing the real meaning in what I did. In the end looks like none of us were sufficiently close to language's low level plumbings dealing with memory initialization. The question is: can this memory be safely reclaimed at all?

The current approach in the fixed branch is to reclaim memory - thus breaking compatibility in some extreme usage cases - while leaving string initialization order and processing as is. Andreas noticed this appeared to be the same of just using std::string objects directly but we weren't sure of the details.

I know this probably does not help much but I'm afraid it's the best we can do without extensive testing nor contacting someone with special knowledge on the subject.

Massimo Del Zotto

[schuhschuh]

Comment #15 originally posted by schuhschuh on 2014-03-20T15:32:01.000Z:

After the release of v2.1 (which mainly contains minor bug fixes and in particular the migration to CMake), I would like to eventually resolve this issue (and others) as part of v2.2.

[schuhschuh]

Incorrectly tagged commits 48e1a44 and cd1f401 related to this issue due to mismatch of issue ID between previous Google Code (#40) and this GitHub project. Updated bugfix branch to incorporated latest changes of master in preparation of final merge of bugfix into master (cf 470b3e8).

[schuhschuh]

Will introduce the changes of @MaxDZ8 with an increase of major version number due to API changes.

[schuhschuh]

Renamed related bugfix branch from "bugfix-40-memory-leaks" to bugfix/#51-memory-leaks to remove confusion about issue number.

[schuhschuh]

I wonder if we should not get rid of the placement-new hack entirely when switching to non-POD string objects for string flags. We anyway break code which uses the string flags in a destructor of a static object. Take also note to include a warning in the documentation regarding this change and that global flag variables are not meant to be accessed in constructors/destructors. One can always pass the value of a flag after parsing the command line as member of a static object to circumvent the need for reading the value during static de-initializaiton. Disallowing such use would also allow us to use STL containers as flag types (cf. #82).

[ppedriana]

Are we going to see a fix for this bug? We are trying to do heap analysis here and this bug is getting in the way. Thanks.

[ppedriana]

Here is a proposed fix memory leak fix, which works in my testing with one compiler. It adds struct gflags_string_free_##name

#define DEFINE_string(name, val, txt)                                       \
  namespace fLS {                                                           \
    using ::fLS::clstring;                                                  \
    static union { void* align; char s[sizeof(clstring)]; } s_##name[2];    \
    clstring* const FLAGS_no##name = ::fLS::                                \
                                   dont_pass0toDEFINE_string(s_##name[0].s, \
                                                             val);          \
    static GFLAGS_NAMESPACE::FlagRegisterer o_##name(                       \
        #name, "string", MAYBE_STRIPPED_HELP(txt), __FILE__,                \
        s_##name[0].s, new (s_##name[1].s) clstring(*FLAGS_no##name));      \
    extern GFLAGS_DLL_DEFINE_FLAG clstring& FLAGS_##name;                   \
    using fLS::FLAGS_##name;                                                \
    clstring& FLAGS_##name = *FLAGS_no##name;                               \
                                                                            \
    struct gflags_string_free_##name{                                       \
        ~gflags_string_free_##name() {                                      \
            clstring* pstr0 = reinterpret_cast<clstring*>(s_##name[0].s);   \
            pstr0->~clstring();                                             \
            clstring* pstr1 = reinterpret_cast<clstring*>(s_##name[1].s);   \
            pstr1->~clstring();                                             \
        }                                                                   \
    };                                                                      \
    gflags_string_free_##name s_gflags_string_free_##name;                  \
  }                                                                         \

[schuhschuh]

@ppedriana Thanks for the patch. You're right that this should fix the string flags memory leaks. Not sure why I only tried to do so in the FlagValue destructor after I figured out that an explicit call of the destructor is needed... Given that the placement new is performed by this macro code, it's reasonable and good to have the corresponding "placement delete" part within the same code snippet/compilation unit.