Wrapper for memory allocators

[antoinealb]

Should we provide some sort of wrapper around malloc() to check for common errors ? Git and Neovim do that too [1]. It would allow to have a centralized error checking place (easier to break on / log) and also fill the allocated memory with garbage when compiling for the tests (see XMALLOC_POISON below).

Example implementation, adaptated from Git :

void *xmalloc(size_t size)
{
    void *ret;

    memory_limit_check(size);
    ret = malloc(size);
    if (!ret && !size)
        ret = malloc(1);
    if (!ret) {
        die("Out of memory, malloc failed (tried to allocate %lu bytes)",
            (unsigned long)size);
    }
#ifdef XMALLOC_POISON
    memset(ret, 0xA5, size);
#endif
    return ret;
}

[1] https://github.com/git/git/blob/master/wrapper.c#L45

[2] https://github.com/philix/neovim/blob/473c93c5fa94b46ba32812b67ce7170f1b8c1dd1/src/misc2.c#L670

[froj]

I think it's a good idea, as we will need to check the return value of malloc every time anyway.

I'm not sure if we should add the memory_limit_check, though.

[31415us]

I think that could be very useful in case we ever want to tinker with different malloc implementations or whatnot. I like the idea of XMALLOC_POSION for testing. Two ideas:

1. Shouldn't memory_limit_check verify that size > 0?
2. Out of curiosity: for unit testing how do you make malloc returnNULL (out of memory)?

[froj]

1. Shouldn't memory_limit_check verify that size > 0?

size_t is unsigned.

[31415us]

1. Shouldn't memory_limit_check verify that size > 0? size_t is unsigned.

yeah but unsigned just means non-negative (= could still be == 0. What i mean is that I don't see the point of the if (!ret && !size) test.

[froj]

malloc(0) is actually a legit call and should by definition return NULL.

Edit: From the mallocman-page:

If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free().

[31415us]

well in this case the

if (!ret && !size)
    ret = malloc(1);

is still useless^^ Since we already try to wrap malloc why not make malloc(0) something that gives you at least a warning? Are there legit cases where allocating 0 bytes is something you want to do?

[froj]

Apparently they want xmalloc to return a non-NULL pointer whenever the function call was successful (even when it was called with size = 0).

A legit case is e.g. when you have a dynamically allocated list and that list happens to be empty. You still try to allocate it, because it's the field of a structure or something that you're initializing. But personally, I'd also check size for zero before calling malloc.

[pierluca]

I'm both strongly in favor of replacing the default malloc and strongly against the idea of rolling our own non-trivial solution. Memory managers can be a pain in the ass to write properly, they're extremely hard to test for bugs (real ones, not simulated) and only non-trivial solutions bring sizeable testing advantages.

If anything, I would suggest two things, combined. (1) A trivial (i.e. evidently bug-free) wrapper around malloc AND free, like // xmemory.h void *xmalloc(size_t size) { void *ret; ret = malloc(size); if (ret) return ret; // log and fail hard } xfree(void* ptr) { free(ptr) }

(2) Relying on the Fluid Studios (Paul Nettle's) Memory Manager (hereafter mmgr) http://www.paulnettle.com/pub/FluidStudios/MemoryManagers/ This is, simply put, the best memory manager you'll find around. It has a huge user base and I seriously doubt any bugs are left in it. Only minor work should be needed to make it ANSI C (as opposed to C++), such as removing the new / delete operators. mmgr outputs a log file with all the memory leaks it found, can randomly fail and can stress test the application by filling the memory with random values (rather than constant ones). It has to be included in every file allocation/deallocating memory, which could be JUST the xmemory.h file, with solution 1.

Why I suggest this? We keep our code simple and only using xmalloc to have a common failure point to debug on. We leave the stress testing and advanced, optional functions to mmgr.h while reducing our own development effort.

---

edit: markdown was doing some weird stuff to my text, sorry about that :)

[pierluca]

Scratch that. Either option 1 or option 2. Both is suboptimal, because the whole memory leak detection function will just point to our own xmemory file rather than to the specific locations.

We might want, however, to have our code compile against either version 1 or version 2 (slightly modified) depending on whether it's the testing or embedded build. It's slightly risky though, because we run again into cross-platform issues.

[31415us]

That mmgr looks quite neat. After a bit of google-fu I couldn't find anything about using it in an embedded environment but some posts stating that it's not working when used in a multithreaded context. That seems like a deal breaker to me, although it might be fixable.

[Stapelzeiger]

I like the idea of having a wrapper for malloc that panics with an "out of memory" message in case malloc returns NULL. It simplifies code and makes sure we always check malloc's return value and fail in a defined manner. But I wouldn't replace the default malloc implementation (dlmalloc I think) of newlib. We will be using malloc sparingly anyway, so I don't think it's worth investing time in integrating another memory allocator. If we see that the default allocator is not sufficient we can still replace it. (TLSF (http://www.gii.upv.es/tlsf/) might be an interesting choice if we need realtime guarantees)

[pierluca]

After a bit of google-fu I couldn't find anything about using it in an embedded environment but some posts stating that it's not working when used in a multithreaded context

It's true, mmgr is not designed for a multithreaded context and I wouldn't try to fix it. It's really too much of an effort for what it's worth and we'll introduce new bugs. I wasn't mentioning it as a full-project memory manager but for specific testing builds (I wasn't clear, sorry). The release-like debugging builds should be using the same memory management function as the real deal anyways... which should be as simple as possible.

I like @Stapelzeiger 's take on this.

[antoinealb]

Implemented in #24