mem_alloc_align() vs. free

[solardiz]

In 2015 @magnumripper's f22ab24 introduced usage of lots of different OS'es aligned malloc functions. Some of these may require freeing the memory differently than we normally do, but we don't have a corresponding mem_free_align wrapper. These are probably dormant bugs.

For example, here's Monero project being more careful about this:
https://github.com/monero-project/monero/blob/13002ddd4b1ba4b4dcd7c47aa368a87c353b2225/src/crypto/slow-hash.c#L1283

STATIC INLINE void* aligned_malloc(size_t size, size_t align)
{
    void *result;
#ifdef _MSC_VER
    result = _aligned_malloc(size, align);
#else
    if (posix_memalign(&result, align, size)) result = NULL;
#endif
    return result;
}

STATIC INLINE void aligned_free(void *ptr)
{
#ifdef _MSC_VER
    _aligned_free(ptr);
#else
    free(ptr);
#endif
}

We also use _aligned_malloc as a last resort:

#elif HAVE__ALIGNED_MALLOC
        if (!(ptr = _aligned_malloc(size, align)))

but there's currently no mention of _aligned_free in our tree.

If we have to introduce a custom free anyway, we could alternatively switch to using our regions API, which we normally only use for large allocations, but there's no reason we can't use it for smaller ones as well - except that we'd need to modify the code to never use mmap below a size threshold. Switching to that API would allow us not to care about OS support for aligned malloc, as the region struct lets us do our own alignment yet separately record the original allocated pointer for freeing. So we could drop some autoconf checks and also be portable to systems that lack aligned malloc.

[solardiz]

there's currently no mention of _aligned_free in our tree.

Oh, I'm wrong, there is in memory.h:

#ifdef _MSC_VER
#define malloc(a) _aligned_malloc(a,16)
#define realloc(a,b) _aligned_realloc(a,b,16)
#define calloc(a,b) memset(_aligned_malloc(a*b,16),0,a*b)
#define free(a) _aligned_free(a)
#define strdup(a) strdup_MSVC(a)
extern char *strdup_MSVC(const char *str);
#define MEM_FREE(ptr) \
{ \
        if ((ptr)) { \
                _aligned_free((ptr)); \
                (ptr) = NULL; \
        } \
}

#elif HAVE___MINGW_ALIGNED_MALLOC
#define malloc(a) __mingw_aligned_malloc(a,(sizeof(long long)))
#define realloc(a,b) __mingw_aligned_realloc(a,b,(sizeof(long long)))
#define calloc(a,b) memset(__mingw_aligned_malloc(a*b,(sizeof(long long))),0,a*b)
#define free(a) __mingw_aligned_free(a)
#define strdup(a) strdup_MSVC(a)
extern char *strdup_MSVC(const char *str);

#define MEM_FREE(ptr) \
{ \
        if ((ptr)) { \
                __mingw_aligned_free((ptr)); \
                (ptr) = NULL; \
        } \
}

#else
#define MEM_FREE(ptr) \
{ \
        if ((ptr)) { \
                free((ptr)); \
                (ptr) = NULL; \
        } \
}
#endif

That's quite a hack I didn't expect. In order to use the same MEM_FREE for aligned and not, we override the main malloc, etc. names turning them into macros and make them always use the aligned versions of the calls. Luckily, we only do this on MSVC and Mingw builds, which we don't fully support (no idea if our current tree builds with those compilers).

[solardiz]

If we have to introduce a custom free anyway, we could alternatively switch to using our regions API

I wrote this thinking there are not a lot of uses of mem_alloc_align in our tree. I now see that while there are not a lot of direct uses of this function, there are a lot of mem_calloc_align, which calls mem_alloc_align. So it'd be many changes throughout the tree, and more lines of code too. Not ideal.

[magnumripper]

Some of these may require freeing the memory differently than we normally do, but we don't have a corresponding mem_free_align wrapper. These are probably dormant bugs.

Given that the MSVC/MinGW cases are taken care of (by JimF I guess), the only remaining issue would be (as described in the comment in memory.c) the obsolete memalign() that may or may not support its memory to be freed at all so our only option would be to silently ignore a MEM_FREE() and leak memory. I have yet to hear about anyone hitting that problem, I expect it to only apply to systems so old they'd be unusable for cracking anyway.

I'm curious though: Why is the hack for MSVC/MinGW so bad? Sure it's not beautiful but does it hurt non-aligned allocs in some way I fail to see?

[magnumripper]

the obsolete memalign() that may or may not support its memory to be freed at all

We could of course drop that choice in the autoconf stuff, leaving user with the #error No suitable alligned alloc found. But that would exclude systems that have a memalign() that does work with free().