Valgrind memcheck: still reachable bytes in Singleton

[chazzmcdaniels]

The new in a Singleton has no complementary delete which causes still reachable bytes in memcheck. (misc.h line 258 and 346)

Code snippet to reproduce (FixedMaxPlaintextLength is the trigger here):

CryptoPP::RSA::PublicKey publicKey{};
....
CryptoPP::RSAES<CryptoPP::OAEP<CryptoPP::SHA256>>::Encryptor encryptor(publicKey);
if (mSessionData.size() > encryptor.FixedMaxPlaintextLength())
...

Environment: Ubuntu 20.04, Crypto++ 8.7.0, gcc 11.1.0, Valgrind 3.15.0 Command line: valgrind --track-origins=yes --leak-check=full --show-leak-kinds=all .......... Expected output: No leaks at all Current output:

==2901338== Memcheck, a memory error detector
==2901338== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==2901338== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==2901338== Command: ..........
==2901338== 
....

OK (4)
==2901338== 
==2901338== HEAP SUMMARY:
==2901338==     in use at exit: 8 bytes in 1 blocks
==2901338==   total heap usage: 2,491,956 allocs, 2,491,955 frees, 96,789,772 bytes allocated
==2901338== 
==2901338== 8 bytes in 1 blocks are still reachable in loss record 1 of 1
==2901338==    at 0x483BE63: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==2901338==    by 0x1E27CC: CryptoPP::NewObject<CryptoPP::OAEP<CryptoPP::SHA256, CryptoPP::P1363_MGF1> >::operator()() const (misc.h:258)
==2901338==    by 0x1E241D: CryptoPP::Singleton<CryptoPP::OAEP<CryptoPP::SHA256, CryptoPP::P1363_MGF1>, CryptoPP::NewObject<CryptoPP::OAEP<CryptoPP::SHA256, CryptoPP::P1363_MGF1> >, 0>::Ref() const (misc.h:346)
==2901338==    by 0x1E209C: CryptoPP::TF_ObjectImplBase<CryptoPP::TF_EncryptorBase, CryptoPP::TF_CryptoSchemeOptions<CryptoPP::TF_ES<CryptoPP::RSA, CryptoPP::OAEP<CryptoPP::SHA256, CryptoPP::P1363_MGF1>, int>, CryptoPP::RSA, CryptoPP::OAEP<CryptoPP::SHA256, CryptoPP::P1363_MGF1> >, CryptoPP::RSAFunction>::GetMessageEncodingInterface() const (pubkey.h:594)
==2901338==    by 0x1E05D8: CryptoPP::TF_CryptoSystemBase<CryptoPP::PK_Encryptor, CryptoPP::TF_Base<CryptoPP::RandomizedTrapdoorFunction, CryptoPP::PK_EncryptionMessageEncodingMethod> >::FixedMaxPlaintextLength() const (pubkey.h:273)
==2901338==    ..........
==2901338== 
==2901338== LEAK SUMMARY:
==2901338==    definitely lost: 0 bytes in 0 blocks
==2901338==    indirectly lost: 0 bytes in 0 blocks
==2901338==      possibly lost: 0 bytes in 0 blocks
==2901338==    still reachable: 8 bytes in 1 blocks
==2901338==         suppressed: 0 bytes in 0 blocks
==2901338== 
==2901338== For lists of detected and suppressed errors, rerun with: -s
==2901338== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

[graphitemaster]

Yep, the problem is the Singleton<T> implementation in https://github.com/weidai11/cryptopp/blob/master/misc.h#L332-L352

Does something like:

static std::atomic<T*> s_pObject;
T *newObject = m_objectFactory(); // where m_objectFactory is `new T`
s_pObject.store(newObject);

But the heap allocated object stored in s_pObject is never deleted on exit. This can be fixed by implementing an atomic_unique_ptr like the following

template<typename T>
struct atomic_unique_ptr {
  using pointer = T *;
  std::atomic<pointer> ptr;
  constexpr atomic_unique_ptr() noexcept : ptr() {}
  explicit atomic_unique_ptr(pointer p) noexcept : ptr(p) {}
  atomic_unique_ptr(atomic_unique_ptr&& p) noexcept : ptr(p.release()) {}
  atomic_unique_ptr& operator=(atomic_unique_ptr&& p) noexcept { reset(p.release()); return *this; }
  void reset(pointer p = pointer()) { auto old = ptr.exchange(p); if (old) delete old; }
  operator pointer() const { return ptr; }
  pointer operator->() const { return ptr; }
  pointer get() const { return ptr; }
  explicit operator bool() const { return ptr != pointer(); }
  pointer release() { return ptr.exchange(pointer()); }
  ~atomic_unique_ptr() { reset(); }
};

Then replacing the static std::atomic<T*> s_pObject with an atomic_unique_ptr<T> as a drop-in replacement, fixing the memory leaks.

[noloader]

On Sun, Oct 15, 2023 at 4:34 AM Dale Weiler @.***> wrote:

Yep, the solution would be to just implement something like atomic_unique_ptr so that the object returned by m_objectFactory would also deleted correctly.

templatestruct atomic_unique_ptr { using pointer = T ; std::atomic ptr; constexpr atomic_unique_ptr() noexcept : ptr() {} explicit atomic_unique_ptr(pointer p) noexcept : ptr(p) {} atomic_unique_ptr(atomic_unique_ptr&& p) noexcept : ptr(p.release()) {} atomic_unique_ptr& operator=(atomic_unique_ptr&& p) noexcept { reset(p.release()); return this; } void reset(pointer p = pointer()) { auto old = ptr.exchange(p); if (old) delete old; } operator pointer() const { return ptr; } pointer operator->() const { return ptr; } pointer get() const { return ptr; } explicit operator bool() const { return ptr != pointer(); } pointer release() { return ptr.exchange(pointer()); } ~atomic_unique_ptr() { reset(); } };

This more or less works as a drop-in replacement and fixes the memory leaks.

Yeah, the problem is (or has always been) C++03. We still support it. I know there are folks still using Crypto++ on antique platforms, like Windows XP and old versions of GCC like 4.4.

To abstract away the differences between auto_ptr (C++03) and unique_ptr (C++11), the library provided member_ptr. The problem is, it is not reference counted. There's always a race in destruction in mutithreaded environments, so the library punted and opted for a memory leak.

For the regularly used stuff, like Integer class, we cutover to a pattern that avoids the leak (some hand waving):

const Integer& Integer::Two() const {

if defined CRYPTOPP_CXX11_STATIC_INIT

    static const Integer two(2);
    return two;

else

    // SIngleton gyrations

endif

}

If P1363_MGF1 is causing an accumulation of leaks for you, then we can look at cutting it over to the new pattern.

Jeff

Message ID: @.***>