Replacing global operator new / delete and allocating memory within these, leading to heap corruption

Question

#include <cstdlib>
#include <memory>
#include <unordered_map>

template <class T>
struct allocator {
  typedef size_t size_type;
  typedef ptrdiff_t difference_type;
  typedef T* pointer;
  typedef const T* const_pointer;
  typedef T& reference;
  typedef const T& const_reference;
  typedef T value_type;

  allocator() = default;

  template <class U>
  allocator(const allocator<U>&) {}

  T* allocate(std::size_t n) const { return (T*)malloc(n); } // debugger breaks here
  void deallocate(T* p, std::size_t) const { free(p); }
};

using allocations_map =
    std::unordered_map<void*, std::size_t, std::hash<void*>,
                       std::equal_to<void*>,
                       allocator<std::pair<void* const, std::size_t>>>;

allocations_map allocations; // heap corruption in the constructor

void* operator new(std::size_t n) {
  auto p = malloc(n);
  allocations.emplace(p, n);
  return p;
}

void operator delete(void* p) noexcept {
  allocations.erase(p);
  free(p);
}

int main() { std::vector<int> v(5); }

Why do i corrupt the heap in the constructor of allocations_map? The debugger detects the first heap corruption in a malloc call of the custom allocator, called inside the constructor.

Is there a more elegant solution then to write a non-logging custom allocator for allocations_map? The container shall obviously not log its own allocations.

I also tried two singleton approaches, as suggested in the comments, without success:

allocations_map& get_allocations_map()
{
    static allocations_map* allocations_ptr = nullptr;
    if (allocations_ptr == nullptr)
    {
        allocations_ptr = (allocations_map*) malloc(sizeof(allocations_map));
        allocations_ptr = new(allocations_ptr)allocations_map;
    }
    return *allocations_ptr;
}


allocations_map& get_allocations_map()
{
    static allocations_map allocations;
    return allocations;
}

This looks like [static initialization order fiasco](https://en.cppreference.com/w/cpp/language/siof). Globals and static members from other translation units are probably being initialized before `allocations`. When these objects try to `new`, the expression `allocations.emplace(p, n);` uses an uninitialized object. You can try using [Meyers' Singleton](https://stackoverflow.com/questions/17712001/how-is-meyers-implementation-of-a-singleton-actually-a-singleton) instead of a global object for `allocations` to get around this problem. — François Andrieux, Oct 29 '20 at 13:58
Not sure if that is part of the problem, but you only allocate `n` bytes for `n` `int`s, that take up (depending on your system) probably 20 to 40 bytes. — n314159, Oct 29 '20 at 14:00
You should also be careful of using this solution in a threaded environment. The map is not synchronized. I'm curious if static initialization is required to occur on a single thread. — François Andrieux, Oct 29 '20 at 14:04
@FrançoisAndrieux Thanks i will test the singleton approach, your explanations sounds good. The real code includes locking ofcourse. — lars, Oct 29 '20 at 14:05
@lars You'll need to make sure any `mutex` or other synchronization primitive you use doesn't also fall into the static initialization order problem. — François Andrieux, Oct 29 '20 at 14:08
I retract my prior comment, red herring. Try this: `T* allocate(std::size_t n) const { std::printf("alloc %s %zu x %zu\n", typeid(T).name(), sizeof(T), n); return static_cast(std::malloc(n)); }` — Eljay, Oct 29 '20 at 14:09

score 2 · Accepted Answer · answered Oct 29 '20 at 15:01

2

From std::allocator::allocate allocator allocates n "things" not n bytes. You should change:

T* allocate(std::size_t n) const { return (T*)malloc(n); }

to:

T* allocate(std::size_t n) const { return (T*)malloc(sizeof(T) * n); }

Why do i corrupt the heap in the constructor of allocations_map?

Because the constructor of elements stored in that map access allocated memory out-of-bounds.

answered Oct 29 '20 at 15:01

KamilCuk

120,984
8
59
111

Thanks, stupid me... Solved. The comment regarding the static intitialization order was also a possible bug, so at least i learned more then the bug fix ;) – lars Oct 29 '20 at 15:06

Replacing global operator new / delete and allocating memory within these, leading to heap corruption

1 Answers1