Is it safe to `free` memory if allocated via overloaded `new[]` that delegates to `malloc`?

Question

My question is not a duplicate of Is it safe to `free()` memory allocated by `new`?.

I'm writing a toy garbage collector for PODs, in which I'm defining my own custom operator new/new[] and operator delete/delete[]. Code below:

#include <iostream>
#include <map>

std::map<void*, std::size_t> memory; // globally allocated memory map
struct collect_t {} collect; // tag for placement new

void* operator new(std::size_t size, const collect_t&)
{
    void* addr = malloc(size);
    memory[addr] = size;
    return addr;
}

void* operator new[](std::size_t size, const collect_t&) 
{
    return operator new(size, collect);
}

void operator delete(void *p, const collect_t&) noexcept
{
    memory.erase(p); // should call ::operator delete, no recursion
    free(p);
}

void operator delete[](void *p, const collect_t&) noexcept
{
    operator delete(p, collect);
}

void display_memory()
{
    std::cout << "Allocated heap memory: " << std::endl;
    for (auto && elem : memory)
    {
        std::cout << "\tADDR: " << elem.first << " "
                  << "SIZE: "  << elem.second << std::endl;
    }
}

void clear()
{
    for (auto && elem : memory)
        free(elem.first); // is this safe for arrays?
    memory.clear();
}

int main()
{
    // use the garbage collector
    char *c = new(collect) char; 
    int *p = new(collect) int[1024]; // true size: sizeof(int)*1024 + y (unknown overhead)

    display_memory();
    clear();
    display_memory();
}

The idea is simple: I store all allocated tracked addresses (the ones allocated with my custom new) in a std::map, and make sure that at the end of the day I clear all memory in my clear() function. I use a tag for my new and delete (and don't overload the global ones) so that std::map's allocator can call the global ones without recurring.

My question is the following: in my clear() function, I de-allocate the memory in the line

for (auto && elem : memory)
    free(elem.first); // is this safe for arrays?

Is this safe for arrays, e.g. for int *p = new(collect) int[1024];?. I believe it is, since void* operator new[](std::size_t size, const collect_t&) calls operator new(size, collect);, and the latter calls malloc. I am not 100% sure though, can something go wrong here?

As long as you make sure that *everybody* uses your operators, including separately built libraries, then yes. Tends to be quite difficult to guarantee. — Hans Passant, Apr 23 '15 at 16:50
It is safe if you do it right: Your placement new is adding additional header information (the size). Hence you need to increase the amount of allocated memory by aligned( sizeof(header)) and return a (aligned) pointer past the header. When deallocating you have to subtract that size first. Further it requires some discipline to use the new/delete pairs. — , Apr 23 '15 at 16:55
Accessing/modifying memory map is not thread safe, but you might not be a requirement. — Tony J, Apr 23 '15 at 16:58
@Tony, yes I know, it's just a toy example I'm using to learn a bit about placement new/delete. — vsoftco, Apr 23 '15 at 17:00
However (!) Don not free the remaining elements in your map, you still need a proper delete, unless you are allocating trivially destructable types, only. — , Apr 23 '15 at 17:02
@DieterLücking not sure I understand your last comment. I delete only the memory the pointers in the map point to. And yes, I assume my types are PODs (or trivially destructible). In this case I believe I don't need the `delete` operator, as I'm not calling any destructor. — vsoftco, Apr 23 '15 at 17:05
@DieterLücking Can you clarify a bit the overhead issue? I know that `operator new[]` may introduce some overhead (additional bytes), however I thought that they are captured by the `size` parameter, so at the end I end up `malloc`-ing the correct size (which includes the overhead). Then why cannot I simply `free` that block and have to substract the overhead? And why the need for an aligned pointer? — vsoftco, Apr 23 '15 at 17:10
@vsoftco: That assumption is wrong. The size passed to the allocating operator new is the size of the object/array (In other words: C++ does not know what extra header memory is required) — , Apr 23 '15 at 17:15
@DieterLücking I'll gladly accept your answer if you can post it, and write some details about the alignment issue. I don't know how to make sure I'm allocating enough memory (in other words, I don't know how to find the extra overhead). — vsoftco, Apr 23 '15 at 17:20
See [this question.](http://stackoverflow.com/q/8720425/315052) — jxh, Apr 23 '15 at 17:34
`void operator delete(void *p, const collect_t&) noexcept` is only called if the constructor of an object created by custom-placement new (such as `new (collect)`) throws. As your design is for pod types only, that should not be possible, as the constructors are vacuous? — Yakk - Adam Nevraumont, Apr 23 '15 at 17:50
@DieterLücking Your concern doesn't seem to apply here. This isn't placement new into a `void*`, but rather custom placement new. The `size_t` passed will to either, by my reading of the standard snippets, include any overhead required? The usual concern with placement new is that the target buffer size is determined *before* calling `new[]`, which could ask for more memory than the buffer has. I am uncertain what to do about alignment, however. In short: the size passed to `new[]` is the size of the memory requested, not the size of the array. — Yakk - Adam Nevraumont, Apr 23 '15 at 17:53
@DieterLücking n4296 [expr.new] 4.3.4/11 and /12 and /13 and /14 -- please provide evidence that my interpretation of those paragraph is incorrect, or that the standard has fundamentally changed? (this comment has been edited) There are explicit statements in the examples that array overhead will be passed in, and it is implied by the other text. What more, if the `size_t` passed to `new[]` doesn't include that overhead, I cannot figure out a way to make `new[]` at all usable. So I'm skeptical of your claim. Did I misunderstand your claim? Am I mistaken? Are you? — Yakk - Adam Nevraumont, Apr 23 '15 at 18:02
@Yakk: I think you are correct. The subtlety is whether or not the argument to placement new has enough space to accept the total array size. In this case, the implementation of the placement new is not being given a pointer to an object to initialize, but a tag to trigger an allocation, so the total size is being used to create a large enough object. — jxh, Apr 23 '15 at 18:14
@Yakk The overhead mentioned there (C++ 11 in 5.3.4) is allowing the compiler to add extra information for array management. However, it is the size required for an object/array, still. If you want your own extra information you have to increase that size by the size your information has. — , Apr 23 '15 at 18:16
@DieterLücking Your reading of the OP's code is incorrect. In the very first post you imply the OP is storing extra information in the allocated memory: that is not true. Your claim that others assumptions where wrong was due to misreading what they are saying: every instance of "that assumption is wrong" you made is incorrect in this comment thread. This statement "The size passed to the allocating operator new is the size of the object/array" you made is blatantly wrong (size of an array is `sizeof(array)`) or misleading. `new[]` **is** passed the size of the array, plus compiler overhead. — Yakk - Adam Nevraumont, Apr 23 '15 at 18:30
@Yakk That is the overhead for maintaining the array, not (!) the overhead for maintaining the allocation! — , Apr 23 '15 at 18:36

score 2 · Answer 1 · answered Apr 23 '15 at 16:55

It appears to me that in order for memory to be in your memory container it must have been allocated with your custom allocator that always calls malloc. Therefore I believe your code calling free should be ok.

Obviously if someone goes around stuffing random addresses into the memory map you will wind up with all sorts of undefined behavior.

jxh · Answer 2 · 2015-04-23T18:20:01.420

Assuming the objects using your garbage collector never implement a destructor, and this holds true for any members that those objects may contain, the code as it were is "safe" in the sense that the call to free() directly is just by-passing the work the compiler would have done to achieve the same thing as it inlined the delete calls.

However, the code is not really safe.

If you ever changed how your garbage collector worked, or how the new function worked, then you would have to hunt down all the direct calls to free() to head off any problems. If the code was ever cut-and-pasted or otherwise reused in a context outside of your garbage collector, you would face a similar problem.

It is just better practice to always match new to delete and malloc to free.

Is it safe to `free` memory if allocated via overloaded `new[]` that delegates to `malloc`?

2 Answers2