Critique my heap debugger

Question

I wrote the following heap debugger in order to demonstrate memory leaks, double deletes and wrong forms of deletes (i.e. trying to delete an array with delete p instead of delete[] p) to beginning programmers.

I would love to get some feedback on that from strong C++ programmers because I have never done this before and I'm sure I've done some stupid mistakes. Thanks!

#include <cstdlib>
#include <iostream>
#include <new>

namespace
{
    const int ALIGNMENT = 16;
    const char* const ERR = "*** ERROR: ";
    int counter = 0;

    struct heap_debugger
    {
        heap_debugger()
        {
            std::cerr << "*** heap debugger started\n";
        }

        ~heap_debugger()
        {
            std::cerr << "*** heap debugger shutting down\n";
            if (counter > 0)
            {
                std::cerr << ERR << "failed to release memory " << counter << " times\n";
            }
            else if (counter < 0)
            {
                std::cerr << ERR << (-counter) << " double deletes detected\n";
            }
        }
    } instance;

    void* allocate(size_t size, const char* kind_of_memory, size_t token) throw (std::bad_alloc)
    {
        void* raw = malloc(size + ALIGNMENT);
        if (raw == 0) throw std::bad_alloc();

        *static_cast<size_t*>(raw) = token;
        void* payload = static_cast<char*>(raw) + ALIGNMENT;

        ++counter;
        std::cerr << "*** allocated " << kind_of_memory << " at " << payload << " (" << size << " bytes)\n";
        return payload;
    }

    void release(void* payload, const char* kind_of_memory, size_t correct_token, size_t wrong_token) throw ()
    {
        if (payload == 0) return;

        std::cerr << "*** releasing " << kind_of_memory << " at " << payload << '\n';
        --counter;

        void* raw = static_cast<char*>(payload) - ALIGNMENT;
        size_t* token = static_cast<size_t*>(raw);

        if (*token == correct_token)
        {
            *token = 0xDEADBEEF;
            free(raw);
        }
        else if (*token == wrong_token)
        {
            *token = 0x177E6A7;
            std::cerr << ERR << "wrong form of delete\n";
        }
        else
        {
            std::cerr << ERR << "double delete\n";
        }
    }
}

void* operator new(size_t size) throw (std::bad_alloc)
{
    return allocate(size, "non-array memory", 0x5AFE6A8D);
}

void* operator new[](size_t size) throw (std::bad_alloc)
{
    return allocate(size, "    array memory", 0x5AFE6A8E);
}

void operator delete(void* payload) throw ()
{
    release(payload, "non-array memory", 0x5AFE6A8D, 0x5AFE6A8E);
}

void operator delete[](void* payload) throw ()
{
    release(payload, "    array memory", 0x5AFE6A8E, 0x5AFE6A8D);
}

Answer 1

void* raw = static_cast<char*>(payload) - ALIGNMENT;

If payload has been already deleted, wouldn't that make this undefined behavior?

Answer 2

I'm not following your use of hardcoded constants/constant strings very well - put them in an enum? And I'm really not getting the idea of the token quite well. Needs more comments

Answer 3

That's a really great start. Here's my 2 cents as you've asked for feedback:

1. The code writes trace information to cerr, which is really for errors. Use cout for informational logs.
2. The ALIGNMENT amount is arbitrary. If code tried to allocate 4090 bytes, you'd allocate 4106, which spills into the next 4k block, which is the size of a memory page. A calculated alignment value would be better... or rename ALIGNMENT as HEADER_SIZE or something similar.
3. Given the header you're creating, you could store the size and a flag for the 'kind of memory' at allocation time and compare it at release time.
4. Token should probably be called 'sentinel' or 'canary value'.
5. Why is Token a size_t? Why not just a void * ?
6. Your check for null in release should probably throw an exception instead - wouldn't it be an error if the code deleted a null pointer?
7. Your 'correct_token' and 'wrong_token' values are far too similar. I had to re-read the code to be sure.
8. Given point (3), you could double the amount you additionally allocate and have before and after sentinels/guard blocks. This would detect memory underrun and overrun.

Answer 4

Explain why you chose "ALIGNMENT" as an identifier. Explain why you picked 16. Argue how your algorithm catches the most common mistakes, like overflowing the end of a heap-allocated block or forgetting to release memory.

Answer 5

Instead of doing intrusive note-keeping you could keep a list of all allocations made. Then you can free the memory without destroying your own data, and keep track of how many times a particular address is "deleted", and also find places where the program tries to delete a non-matching address (i.e. not in the list).