6
\$\begingroup\$

A student was having problems with memory management so I put together an overload of global operator new and global operator delete to explicitly check for memory leaks and freeing invalid memory. This uses boost/stacktrace.hpp to point out exactly where you messed up. It is just for educational and possibly debugging purposes:

#include <boost/stacktrace.hpp> // boost::stacktrace
#include <cstdio>               // fprintf
#include <type_traits>          // decay_t
#include <unordered_map>        // unordered_map
#include <utility>              // move

// An allocator that uses malloc as its memory source
#include "MallocAllocator.h"

// Hash map that uses malloc to avoid infinite recursion in operator new
template<typename Key, typename T>
using MallocHashMap =
  std::unordered_map<Key,
                     T,
                     std::hash<Key>,
                     std::equal_to<Key>,
                     MallocAllocator<std::pair<const Key, T>>>;

// Stack trace that uses malloc to avoid infinite recursion in operator new
using MallocStackTrace = boost::stacktrace::basic_stacktrace<
  MallocAllocator<boost::stacktrace::frame>>;

// Memory allocation info with, address, size, stackTrace
struct MemoryAllocation
{
  // the constructor is so that we can use emplace in unordered_map
  MemoryAllocation(void const* const address_,
                   std::size_t const size_,
                   MallocStackTrace trace_) noexcept
    : address{ address_ }
    , size{ size_ }
    , stackTrace{ std::move(trace_) }
  {}
  void const* address;
  std::size_t size;
  MallocStackTrace stackTrace;
};

// new/delete -> Object vs new[]/delete[] -> Array
enum class AllocationType
{
  Object,
  Array,
};

// returns AllocationType::Object for AllocationType::Array and
// AllocationType::Array for AllocationType::Object
static constexpr AllocationType
otherAllocType(AllocationType const at) noexcept
{
  switch (at) {
    case AllocationType::Object:
      return AllocationType::Array;
    case AllocationType::Array:
      return AllocationType::Object;
  }
}

// Keeps track of the memory allocated and freed.
template<AllocationType at>
static MallocHashMap<void*, MemoryAllocation>&
get_mem_map()
{
  struct Mem // wrapper around std::unordered_map to check for memory leaks in
             // destructor
  {
    std::decay_t<decltype(get_mem_map<at>())> map; // return type

    ~Mem() noexcept
    {
      if (!map.empty()) { // If map isn't empty, we've leaked memory.
        for (auto const& p : map) {
          auto const& memAlloc = p.second;
          std::fprintf(stderr,
                       "-------------------------------------------------------"
                       "----------\n"
                       "Memory leak! %zu bytes. Memory was allocated in\n%s"
                       "-------------------------------------------------------"
                       "----------\n\n",
                       memAlloc.size,
                       to_string(memAlloc.stackTrace).c_str());
        }
      }
    }
  };

  static Mem mem;
  return mem.map;
}

// test if the memory was allocated with new or new[]
template<AllocationType at>
static bool
allocatedAs(void* const ptr)
{
  auto& memmap = get_mem_map<at>();
  auto const it = memmap.find(ptr);
  return it != memmap.end();
}

template<AllocationType at>
static void* // template for new and new[]
new_base(std::size_t const count)
{
  void* p = std::malloc(count);

  if (!p)
    throw std::bad_alloc{};

  auto& mem_map = get_mem_map<at>();

  mem_map.emplace(std::piecewise_construct,
                  std::forward_as_tuple(p),
                  std::forward_as_tuple(p, count, MallocStackTrace{ 3, 100 }));
  return p;
}

template<AllocationType at>
static void // template for delete and delete[]
delete_base(void* const ptr) noexcept
{
  if (ptr) {                     // it's fine to delete nullptr
    if (!allocatedAs<at>(ptr)) { // check for invalid memory free
      std::fprintf(
        stderr,
        "-----------------------------------------------------------------\n"
        "Invalid memory freed memory at %p\n",
        ptr);
      if (allocatedAs<otherAllocType(at)>(ptr)) {
        auto constexpr myNew = at == AllocationType::Object ? "" : "[]";
        auto constexpr otherNew = at == AllocationType::Array ? "" : "[]";

        std::fprintf(
          stderr,
          "\tNote: this memory was allocated with `new%s` but deleted with "
          "`delete%s` instead of `delete%s`\n",
          otherNew,
          myNew,
          otherNew);
      }
      std::fprintf(
        stderr,
        "%s\n---------------------------------------------------------"
        "--------\n\n",
        to_string(MallocStackTrace{ 3, 100 }).c_str());

      std::_Exit(-1); // If we've messed up, exit
    }

    std::free(ptr);               // free the pointer
    get_mem_map<at>().erase(ptr); // remove from the map
  }
}

void*
operator new(std::size_t const count)
{
  return new_base<AllocationType::Object>(count);
}

void*
operator new[](std::size_t const count)
{
  return new_base<AllocationType::Array>(count);
}

void
operator delete(void* const ptr) noexcept
{
  delete_base<AllocationType::Object>(ptr);
}

void
operator delete[](void* const ptr) noexcept
{
  delete_base<AllocationType::Array>(ptr);
}

MallocAllocator is a simple allocator that uses malloc to avoid operator new calling itself recursively for it's own bookkeeping purposes. Here's MallocAllocator.h:

#ifndef MALLOC_ALLOCATOR_H
#define MALLOC_ALLOCATOR_H
#pragma once // Won't hurt

#include <cstdlib>   // malloc/free
#include <stdexcept> // bad_alloc

template<class T>
struct MallocAllocator
{
        using size_type = std::size_t;
        using difference_type = std::ptrdiff_t;
        using value_type = T;

        MallocAllocator() noexcept = default;
        MallocAllocator(const MallocAllocator&) noexcept = default;
        template<class U>
        MallocAllocator(const MallocAllocator<U>&) noexcept
        {}
        ~MallocAllocator() noexcept = default;

        T* allocate(size_type const s, void const* = nullptr) const
        {
                if (s == 0)
                        return nullptr;

                T* temp = static_cast<T*>(std::malloc(s * sizeof(T)));

                if (!temp)
                        throw std::bad_alloc();

                return temp;
        }
        void deallocate(T* p, size_type) const noexcept { std::free(p); }
};

#endif // !MALLOC_ALLOCATOR_H

I am well aware that this is not thread-safe because of the static map. This should be enough for simple debugging and educational purposes, but I'd be happy to hear if you have comments about improving thread-safety.

\$\endgroup\$
11
  • \$\begingroup\$ Just curious: why #pragma once // Won't hurt + manual include guards? \$\endgroup\$
    – L. F.
    Mar 8 '20 at 2:45
  • \$\begingroup\$ @L. F. It speeds up the compilation a tiny bit sometimes. stackoverflow.com/a/1144110/10147399 \$\endgroup\$ Mar 8 '20 at 8:37
  • 1
    \$\begingroup\$ You do know that Valgrind is already a much more capable memory allocation debugger (and more)? \$\endgroup\$ Mar 9 '20 at 9:20
  • \$\begingroup\$ @TobySpeight not if you are on Windows... \$\endgroup\$ Mar 9 '20 at 11:38
  • 1
    \$\begingroup\$ @Ayxan Maybe most users "you" know. I would argue that it is a minor platform compared to Linux. Most people I know build services and Windows machines are non existing in all of the work places I have worked in 15 years. (Note: I am not saying that windows is not used. I am pointing out my view is as biased as yours because of what I see around me :-) ) \$\endgroup\$ Mar 9 '20 at 20:58
4
\$\begingroup\$
MallocAllocator(const MallocAllocator&) noexcept = default;
//...
~MallocAllocator() noexcept = default;

These two will be defined implicitly anyway. I suggest following the rule-of-zero and not declaring them at all. It will not make a difference here, but as general rule this is relevant, because e.g. the declared destructor inhibits declaration of the implicit move operations.


The Allocator requirements require that MallocAllocator<T> and MallocAllocator<U> be comparable with == and != and that the comparison indicates whether the two instances can be used to deallocate the other's allocations. So you should add:

template<class T, class U>
bool operator==(MallocAllocator<T> const&, MallocAllocator<U> const&) noexcept
{
    return true;
}

template<class T, class U>
bool operator!=(MallocAllocator<T> const&, MallocAllocator<U> const&) noexcept
{
    return false;
}

(or similar)


You probably should add

using propagate_on_container_move_assignment = std::true_type;

to MallocAllocator. Otherwise containers cannot generally statically assert that moving a container with this allocator does not require reallocation.


static constexpr AllocationType
otherAllocType(AllocationType const at) noexcept
{
  switch (at) {
    case AllocationType::Object:
      return AllocationType::Array;
    case AllocationType::Array:
      return AllocationType::Object;
  }
}

This function is ill-formed in C++11, because control flow statements like switch were not allowed in constexpr functions. You would need to rewrite it as single return statement using the conditional operator.


std::decay_t<decltype(get_mem_map<at>())> map;

This is also ill-formed in C++11, because the _t helpers for type traits were only introduced in C++14. So use

typename std::decay<decltype(get_mem_map<at>())>::type map;

instead. In either case this looks very awkward to me and I would probably rather alias the type and type it twice, or if you decide to drop the C++11 support in favor of C++14, you might want to consider making the return type auto& and spelling out the type for map instead.


      if (!map.empty()) { // If map isn't empty, we've leaked memory.

The test is redundant. I don't think it really improves readability either.


MallocStackTrace{ 3, 100 }

These are magic constants that should be defined as named variables instead. Probably it would also be a good idea to wrap this construction into a function that doesn't take any parameters or has defaulted parameters, given that it is used twice.


Your approach using a local static to construct and destruct the allocation map is not always safe when other static storage duration objects are used. Consider for example the test program:

std::vector x;

int main() {
    x.push_back(1);
}

The initialization of x will (dependent on the implementation) probably not call operator new, because no allocation is needed yet. Then push_back requires allocation and calls operator new for the first time, constructing the corresponding static Mem mem;.

Now x's construction has completed before mem's and so mem will be destroyed before x will. This causes the allocation done by x to be reported as memory leak and it also causes undefined behavior when the destructor of x is called, because it will access mem when its lifetime has already ended.

I am not sure whether there is any way to completely avoid this though.

The best I can think of is to mimic #include<iostream>'s behavior and require that all translation units include a header file at the beginning which contains dummy global static objects that call get_mem_map for both allocation types in their initializer, but even then there may be static storage duration objects with unordered dynamic initialization, e.g. in class template specializations, which may execute before those.


For thread-safety, assuming performance isn't really important, I would suggest simply to use a single global mutex and to scope-lock it in both delete_base and new_base.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Hi walnut, welcome to Code Review! \$\endgroup\$
    – L. F.
    Mar 8 '20 at 4:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.