[Bug]: valgrind reports still reachable memory leaks when StrictMock or NiceMock are used

[bitrunner]

Describe the issue

Valgrind reports still reachable memory leaks when StrictMock or NiceMock are used. This didn't happen in gtest/gmock 1.11.0.

Steps to reproduce the problem

• Save this to a file and build it (I used g++ -g gmock_leak.cpp -lgtest -lgmock -lgmock_main):

#include <gmock/gmock.h>
#include <gtest/gtest.h>
struct A { virtual ~A() {} virtual void do_it() {} };
struct MockA: public A { MOCK_METHOD(void, do_it, (), (override)); };
TEST(Test, PleaseDoNotLeak) { ::testing::StrictMock<MockA> mock; }

• Run the resultant executable under valgrind with full leak checking enabled (I used valgrind --track-origins=yes --leak-check=full --show-leak-kinds=all ./a.out)

What version of GoogleTest are you using?

1.12.1

What operating system and version are you using?

debian testing (aka 12, aka bookworm)

What compiler and version are you using?

COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/12/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none:amdgcn-amdhsa
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 12.2.0-11' --with-bugurl=file:///usr/share/doc/gcc-12/README.Bugs --enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++,m2 --prefix=/usr --with-gcc-major-version-only --program-suffix=-12 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-plugin --enable-default-pie --with-system-zlib --enable-libphobos-checking=release --with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch --disable-werror --enable-cet --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none=/build/gcc-12-gRbBs2/gcc-12-12.2.0/debian/tmp-nvptx/usr,amdgcn-amdhsa=/build/gcc-12-gRbBs2/gcc-12-12.2.0/debian/tmp-gcn/usr --enable-offload-defaulted --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 12.2.0 (Debian 12.2.0-11)```

### What build system are you using?

I'm using the package distributed with debian, libgtest-dev:amd64 version 1.12.1-0.2.  So I didn't build it myself.

### Additional context

I'm using valgrind version 3.19.0.

Here's the valgrind output from the simple test program from the "steps to reproduce" section:
```==19050== HEAP SUMMARY:
==19050==     in use at exit: 160 bytes in 2 blocks
==19050==   total heap usage: 187 allocs, 185 frees, 112,822 bytes allocated
==19050==
==19050== 56 bytes in 1 blocks are still reachable in loss record 1 of 2
==19050==    at 0x4840F2F: operator new(unsigned long) (vg_replace_malloc.c:422)
==19050==    by 0x14E471: testing::(anonymous namespace)::UninterestingCallReactionMap()
==19050==    by 0x157E9C: testing::(anonymous namespace)::SetReactionOnUninterestingCalls(unsigned long, testing::internal::CallReaction)
==19050==    by 0x118A71: testing::internal::StrictMockImpl<MockA>::StrictMockImpl() (gmock-nice-strict.h:138)
==19050==    by 0x117D84: testing::StrictMock<MockA>::StrictMock() (gmock-nice-strict.h:242)
==19050==    by 0x1166E1: Test_PleaseDoNotLeak_Test::TestBody() (gmock_leak.cpp:5)
==19050==    by 0x14D486: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
==19050==    by 0x13DE7D: testing::Test::Run()
==19050==    by 0x13E034: testing::TestInfo::Run()
==19050==    by 0x13E5D8: testing::TestSuite::Run()
==19050==    by 0x14397E: testing::internal::UnitTestImpl::RunAllTests()
==19050==    by 0x14D9F6: bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)
==19050==

==19050== 104 bytes in 1 blocks are still reachable in loss record 2 of 2
==19050==    at 0x4840F2F: operator new(unsigned long) (vg_replace_malloc.c:422)
==19050==    by 0x15A7C9: std::_Hashtable<unsigned long, std::pair<unsigned long const, testing::internal::CallReaction>, std::allocator<std::pair<unsigned long const, testing::internal::CallReaction> >, std::__detail::_Select1st, std::equal_to<unsigned long>, std::hash<unsigned long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_rehash(unsigned long, unsigned long const&)
==19050==    by 0x15A965: std::_Hashtable<unsigned long, std::pair<unsigned long const, testing::internal::CallReaction>, std::allocator<std::pair<unsigned long const, testing::internal::CallReaction> >, std::__detail::_Select1st, std::equal_to<unsigned long>, std::hash<unsigned long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_insert_unique_node(unsigned long, unsigned long, std::__detail::_Hash_node<std::pair<unsigned long const, testing::internal::CallReaction>, false>*, unsigned long)
==19050==    by 0x157F73: testing::(anonymous namespace)::SetReactionOnUninterestingCalls(unsigned long, testing::internal::CallReaction)
==19050==    by 0x118A71: testing::internal::StrictMockImpl<MockA>::StrictMockImpl() (gmock-nice-strict.h:138)
==19050==    by 0x117D84: testing::StrictMock<MockA>::StrictMock() (gmock-nice-strict.h:242)
==19050==    by 0x1166E1: Test_PleaseDoNotLeak_Test::TestBody() (gmock_leak.cpp:5)
==19050==    by 0x14D486: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
==19050==    by 0x13DE7D: testing::Test::Run()
==19050==    by 0x13E034: testing::TestInfo::Run()
==19050==    by 0x13E5D8: testing::TestSuite::Run()
==19050==    by 0x14397E: testing::internal::UnitTestImpl::RunAllTests()
==19050==
==19050== LEAK SUMMARY:
==19050==    definitely lost: 0 bytes in 0 blocks
==19050==    indirectly lost: 0 bytes in 0 blocks
==19050==      possibly lost: 0 bytes in 0 blocks
==19050==    still reachable: 160 bytes in 2 blocks
==19050==         suppressed: 0 bytes in 0 blocks```

[terokinnunen]

I got the same. I am using from sources, so I was able to git bisect that problematic commit is 0320f517fd920866d918e564105d68fd4362040a. Valgrind error no longer reported for me if I revert this commit from current main branch latest.

[higher-performance]

If I understand correctly, this appears to be due to the change to g_uninteresting_call_reaction in https://github.com/google/googletest/commit/0320f517fd920866d918e564105d68fd4362040a#r85239553. The object pointed to is no longer destructed upon exiting from the program, thus the leak reported by Valgrind. However, given the object persists permanently from the beginning of the program to the end in both cases, the actual memory usage would not be any different before vs. after that commit.

Would you mind clarifying if the "leakage" is actually causing an execution problem for you somehow, or whether you are simply trying to reduce the noise? If it is the former, please let us know how you are running into the issue (a repro would be very helpful). If it's the latter, then unfortunately we may leave the state as-is.

For context: one reason to lazy-initialize variables like this is the static initialization order fiasco. Unfortunately, while construction can be deferred to occur lazily, the same thing cannot be said for destructors, which suffer from the same problems. Thus ensuring safe tear-down can be difficult if note impossible to ensure, and often the fix is to simply let the variables persist until the process is torn down.

[bitrunner]

Would you mind clarifying if the "leakage" is actually causing an execution problem for you somehow, or whether you are simply trying to reduce the noise?

Before the change Tero pointed out, valgrind didn't report any problems within gtest or gmock. After that change, it does. I would not classify this issue as "trying to reduce the noise". It is also not causing an "execution problem" I'm aware of.

If you decide to do nothing about this, anyone that runs a gtest/gmock based unit test under valgrind that uses StrictMock or NiceMock and wants to have a clean bill of health will have to expend effort investigating these leaks and eventually devise and implement their own mitigation (valgrind suppression, gmock hack, lock into gtest/gmock version 1.11.0, etc).

It looks like Tero and I aren't the only people who have tripped over this so far as that commit has an unanswered question about the justification for introduction of a memory leak from someone else.

I went back to gtest/gmock 1.11.0 to hastily avoid this problem. Building it and my unit test using GCC 12.2.0 with -Wall (which turns on -Wmaybe-uninitialized) does not produce the uninitialized warning that lead to the change that caused this issue. So, minimally, it may be worthwhile to determine if the problem that lead to that change is still relevant.

[derekmauro]

This has nothing to do with "reducing noise". Noise is really bad when it comes to automated testing.

This is actually about the definition of a leak. One definition of a leak is that every new (or malloc() and friends) has to be paired with a delete (or free()) before the program finishes. However, none of the leak checkers I am familiar with use this definition in their default configuration. gperftools actually refers to this as the "draconian" definition of a leak. LLVM's leak sanitizer doesn't support the draconian definition at all. And apparently Valgrind requires --leak-check=full to consider this a leak.

The definition these leak checkers are using by default is something like "it is a leak if there is no live pointer variable that points to the allocation". In the commit in question, at shutdown, a static pointer still points to the memory. If, for example, there were a second new that were assigned to the pointer with deleting the original allocation, the first allocation would be considered a leak since no variable would point to it anymore.

We understand that there are some that prefer the draconian definition of a leak. We disagree and our experience has been that the draconian definition simply isn't useful. C++'s well-known undefined initialization and destruction ordering issues cause far more issues. This is why we don't support draconian leak checking.