I’ll admit, I’m still very new to C++, even though I’ve worked on and off with it over many years. It’s different when you are sitting down and writing a large scale program than just writing small tools or reviewing other peoples tools/code.
I wanted to make sure after I got my reliability layer in that I wasn’t accidentally leaking memory anywhere. It will get progressively harder to identify these leaks the more complex the system becomes, so now is a good time (well, from the start is the best time!) to start checking for issues.
Just because you are using smart pointers, doesn’t mean you are safe from leaking memory!
Enter Valgrind
If you’ve never heard of Valgrind, it’s a tool to do dynamic analysis on your running code. It’s great for detecting memory management and threading bugs. Setup was easy, just your usual ./configure && make && sudo make install deal.
I then ran it against my test suite to see where I stood. After a minute or so (it slows down runtime significantly as it’s hooking all allocation/deallocation calls) I was dumped a huge list of problems in my code, and a summary of all the bytes that were lost:
==2394== LEAK SUMMARY:
==2394== definitely lost: 2,014,736 bytes in 268,512 blocks
==2394== indirectly lost: 1,053,888 bytes in 2,978 blocks
==2394== possibly lost: 4 bytes in 1 blocks
==2394== still reachable: 0 bytes in 0 blocks
==2394== suppressed: 0 bytes in 0 blocks
OK I clearly had some work to do, let’s split them up for each bug:
Bug #1 ‘new’ is old
This one surprised me the most, as I thought I was pretty disciplined in my use of smart pointers:
==2394== 4 bytes in 1 blocks are possibly lost in loss record 1 of 71
==2394== at 0x483E723: operator new[](unsigned long) (vg_replace_malloc.c:725)
==2394== by 0x56EB19: serialization::UInt32tToUChar(unsigned int) (simple.h:165)
==2394== by 0x56ECAD: crypto::Cryptor::Encrypt(unsigned int, std::array<unsigned char, 32ul> const&, std::array<unsigned char, 24ul>&, unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char> >&, unsigned long long) (cryptor.cpp:130)
==2394== by 0x593E3C: net::ReliableEndpoint::SendPacket(Logging::Logger&, crypto::Cryptor&, unsigned char const*, int, Game::Message::MessageType) (reliable_endpoint.cpp:44)
==2394== by 0x53CF27: CATCH2_INTERNAL_TEST_10() (reliability.cpp:549)
==2394== by 0x6DEDD2: Catch::TestInvokerAsFunction::invoke() const (catch_test_case_registry_impl.cpp:149)
==2394== by 0x6D401B: Catch::TestCaseHandle::invoke() const (catch_test_case_info.hpp:115)
==2394== by 0x6D25F6: Catch::RunContext::invokeActiveTestCase() (catch_run_context.cpp:535)
==2394== by 0x6D1044: Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (catch_run_context.cpp:498)
==2394== by 0x6D0753: Catch::RunContext::runTest(Catch::TestCaseHandle const&) (catch_run_context.cpp:235)
==2394== by 0x67D500: Catch::(anonymous namespace)::TestGroup::execute() (catch_session.cpp:110)
==2394== by 0x67C5EB: Catch::Session::runInternal() (catch_session.cpp:332)
Why the heck is serialization::UInt32tToUChar(unsigned int) (simple.h:165) calling new[]?!? I had mentally banned that keyword from my code base, or so I thought, there it was, staring at me:
inline static std::unique_ptr<unsigned char *> UInt32tToUChar(const uint32_t Input)
{
auto Out = new unsigned char[sizeof(uint32_t)];
Out[3] = (Input >> 24);
Out[2] = (Input >> 16);
Out[1] = (Input >> 8);
Out[0] = (Input);
return std::make_unique<unsigned char*>(Out);
}
I legit don’t even remember writing this code: auto Out = new unsigned char[sizeof(uint32_t)];.
Apparently I did this for a few other serialization functions. Anyways, fix was easy, now I just take a pointer from the caller to assign the values so I don’t have to worry about returning anything.
inline static void UInt32tToUChar(const uint32_t Input, unsigned char *Out)
{
Out[3] = (Input >> 24);
Out[2] = (Input >> 16);
Out[1] = (Input >> 8);
Out[0] = (Input);
}
Bug #2 std::vector.clear() does not do what I thought
This one was by far the biggest head scratcher. In the SequenceBuffer class which is used to store packets and their sequence data I resize the vector on construction:
SequenceBuffer(int NumberOfEntries) : NumEntries(NumberOfEntries)
{
EntrySequence.resize(NumberOfEntries);
EntryData.resize(NumberOfEntries);
Reset();
};
/**
* @brief Reset's our buffer, setting our sequence ID back to 0,
* Filling the buffer with 0xFFFFFFFF's to signal they are unused and
* clearing our EntryData vector
*
*/
void Reset()
{
Sequence = 0;
std::fill(EntrySequence.begin(), EntrySequence.end(), 0xFFFFFFFF);
EntryData.clear();
}
After which I reset the data to a known empty/unset state. Everything seems fine, yet looking at Valgrind output I see:
==2394== 5,504 (5,120 direct, 384 indirect) bytes in 16 blocks are definitely lost in loss record 43 of 71
==2394== at 0x483CF83: operator new(unsigned long) (vg_replace_malloc.c:483)
==2394== by 0x5981B3: __gnu_cxx::new_allocator<std::_Sp_counted_ptr_inplace<net::ReliableMessage, std::allocator<net::ReliableMessage>, (__gnu_cxx::_Lock_policy)2> >::allocate(unsigned long, void const*) (new_allocator.h:115)
==2394== by 0x598123: allocate (allocator.h:173)
==2394== by 0x598123: ...
==2394== by 0x596510: SequenceBuffer<net::ReliableMessage>::Insert(unsigned short) (sequencebuffer.h:194)
==2394== by 0x593C01: net::ReliableEndpoint::SendPacket(Logging::Logger&, crypto::Cryptor&, unsigned char const*, int, Game::Message::MessageType) (reliable_endpoint.cpp:16)
==2394== by 0x538ACB: CATCH2_INTERNAL_TEST_6() (reliability.cpp:367)
This happens in every call to Insert():
std::shared_ptr<T> Insert(const uint16_t InsertSequenceId)
{
// ...
int Index = InsertSequenceId % NumEntries;
EntrySequence[Index] = InsertSequenceId;
EntryData[Index] = std::make_shared<T>();
return EntryData[Index];
};
At first I thought it was because of my use of std::shared_ptr<T>. Giving it some more thought I realized these don’t need to be shared pointers, and I changed it to a unique_ptr and simply return a raw pointer instead, knowing that the lifetime of the pointer will prevail longer in the SequenceBuffer.
T* Insert(const uint16_t InsertSequenceId)
{
...
return EntryData[Index].get()
}
I re-ran valgrind but got the same memory leak! What is going on?
Creating a small reproducible test
To troubleshoot better I decided to write a simple test case where I can see the destructor being called:
class Wtf {
public:
~Wtf()
{
cout << "WTF DESTRUCTOR CALLED" << endl;
X = nullptr;
}
auto X = std::make_unique<std::string>("hi");
};
TEST_CASE( "test mem leak", "[container]")
{
SequenceBuffer<Wtf> Buff{1};
auto X = Buff.Insert(0);
cout << X->X << endl;
// Destructor never called
}
OK, why is this ~Wtf() never being called? I started commenting out various parts of the constructor to see if there was something wrong with my vector.
SequenceBuffer(int NumberOfEntries) : NumEntries(NumberOfEntries)
{
//EntrySequence.resize(NumberOfEntries);
//EntryData.resize(NumberOfEntries);
//Reset();
};
...
void Reset()
{
Sequence = 0;
std::fill(EntrySequence.begin(), EntrySequence.end(), 0xFFFFFFFF);
EntryData.clear();
}
I re-ran my test, and to my surprise, the destructor of Wtf was now called! I uncommented resize(NumberOfEntries) and still, the destructor was called. I uncommented Reset and no longer was it called!
I created this minimal test in godbolt to see the disassembly, and I’ll be honest, the assembly code looked almost exactly the same, except different registers being used! Baffling! What exactly does clear() do anyways? Let’s read the documentation of clear():
Erases all elements from the container. After this call, size() returns zero.
Invalidates any references, pointers, or iterators referring to contained elements. Any past-the-end iterators are also invalidated.
Leaves the capacity() of the vector unchanged (Note: the standard’s restriction on the changes to capacity is in the specification of
vector::reserve, see [1]).
After being completely stuck, I started asking around, one of the discords I was in had a very helpful person who patiently explained that size != capacity. I was clearly conflating the two, and thought that clear() acted more like a memset(...,...,0). But what it really does is just set the size to 0. Since I was doing this in the constructor, the capacity was there, but the vector had no idea I was randomly stuffing in elements via the EntryData[Index] = std::make_unique<T>() index operations! This is basically an out of bounds write.
If I had used emplace/push_back etc, it would have properly reset the size, but using [] doesn’t do this. Thus I was sticking my elements into a vector, and it had no idea they were there, so it never bothered to call their destructors at cleanup time.
The fix was to no longer call clear() after resize, but also replace all instances of [] operations with vector.at(Index) so I would at least get an exception:
T* Insert(const uint16_t InsertSequenceId)
{
// ...
int Index = InsertSequenceId % NumEntries;
EntrySequence.at(Index) = InsertSequenceId;
EntryData.at(Index) = std::make_unique<T>();
return EntryData.at(Index).get();
};
Re-running valgrind I went from:
==14942== LEAK SUMMARY:
==14942== definitely lost: 954,480 bytes in 3,458 blocks
==14942== indirectly lost: 1,053,888 bytes in 2,978 blocks
==14942== possibly lost: 0 bytes in 0 blocks
==14942== still reachable: 0 bytes in 0 blocks
==14942== suppressed: 0 bytes in 0 blocks
down to:
==24275== LEAK SUMMARY:
==24275== definitely lost: 120 bytes in 6 blocks
==24275== indirectly lost: 192 bytes in 2 blocks
==24275== possibly lost: 0 bytes in 0 blocks
==24275== still reachable: 0 bytes in 0 blocks
==24275== suppressed: 0 bytes in 0 blocks
Good progress!
Bug #3 ‘SmartPointer.release()’ does not do what I thought
Starting to see a pattern here? Yeah I need to read the documentation closer! If you remember a while back I wrote a post titled DLLs Memory and You in which I needed to have my smart pointers that were allocated in the pmo library also be released in the library otherwise I’d get heap corruption.
My brilliant fix was:
static inline void ReleaseSocketMessage(std::unique_ptr<SocketMessage> Message)
{
Message.release();
}
After adding this, and calling it from the exectuable, I no longer got heap corruption!
Turns out the reason I no longer got heap corruption is because I had a memory leak, and those messages were never deallocated properly.
Why did I think release() would deallocate? Let’s look at the intellisense popup:
“Release ownership of any stored pointer” I don’t know why I thought that would call the destructor and “release the memory” but it releases the ownership, meaning it’s no longer a smart pointer that can’t tell that it’s out of scope / should be destroyed.
This one took some debugging as well, similar to bug #2 I made a small reproducible test case. I first thought it was an issue with my LockFreeQueue but using the Wtf class I realized it was properly calling the destructor. I finally changed Message.release() to Message = nullptr and finally I get:
==6563== HEAP SUMMARY:
==6563== in use at exit: 0 bytes in 0 blocks
==6563== total heap usage: 25,404 allocs, 25,404 frees, 62,683,387 bytes allocated
==6563==
==6563== All heap blocks were freed -- no leaks are possible
Tada~
Back to reliability in UDP I go…
a place to jot 