Lameguy64 / PSn00bSDK

The most powerful open source SDK for the PS1 (as far as open source PS1 SDKs go). Not recommended for beginner use.
Other
838 stars 68 forks source link

Realloc does not correctly request sized block with malloc in fresh-alloc-copy state. #78

Closed EngineersBox closed 9 months ago

EngineersBox commented 9 months ago

During the invocation of realloc, the last case for resizing a block to a new given size is done via a call to malloc(...) and then a subsequent memcpy(...) between the old and new blocks.

However, the invocation of malloc is passed the _size parameter instead of size which already has the header bytes added:

size_t      _size = _align(size + sizeof(BlockHeader), 8); // <-- Full memory block size calculated
BlockHeader *prev = (BlockHeader *) ((uintptr_t) ptr - sizeof(BlockHeader));

// ... snip .. other resizing cases

// No luck.
void *new = malloc(_size); // <-- This is incorrect, has sizeof(BlockHeader) additionally, should be size
if (!new) return 0;

__builtin_memcpy(new, ptr, prev->size);
free(ptr);
return new;

The issue is that malloc(...) also adds the sizeof(BlockHeader) padding to a requested size:

__attribute__((weak)) void *malloc(size_t size) {
    if (!size) return 0;

    // Extra sizeof(BlockHeader) padding added here
    size_t _size = _align(size + sizeof(BlockHeader), 8);

    // ... snip ... other malloc code
}

So an initial request size of 92 bytes plus the 16 bytes for the header, aligned to 8 bytes is 112 bytes total. Passing that to malloc, 112 bytes plus the 16 bytes for the header, aligned to 8 bytes is 128 bytes.

So we have over allocated a total of 16 bytes:

#include <stdio.h>

#define _align(x, n) (((x) + ((n) - 1)) & ~((n) - 1))

int main() {
    int req = 92 + 16;
    int _size = _align(req, 8);
    int next_size = _size + 16;
    int m_size = _align(next_size, 8);

    printf("%d => %d, %d => %d\n", req, _size, next_size, m_size);

    return 0;
}

Result: 108 => 112, 128 => 128.

Assuming that everything I've stated is correct (happy to be corrected if not), then this would be good for a PR to fix, a simple one liner at that.

spicyjpeg commented 9 months ago

Thank you for finding this bug. realloc() is indeed allocating slightly more memory than it should; it's not a particularly big deal as the overhead is minimal and the extra memory is going to be freed properly once the block is deallocated, but I've fixed it anyway as part of the latest commit. I have reported the bug to @nicolasnoble as well, as it's also present in the original implementation of this allocator.