arc80/plywood

ChunkCursor::toString does a PLY_HEAP.realloc on an aligned allocation

Closed this issue · 2 comments

When doing a NativePath::join with Heap_CRT in debug, I get an error saying:

The Block at 0x0000024D5BA44B30 was allocated by aligned routines, use _aligned_realloc()

Full callstack is:

>	realloc_dbg_nolock(void * const block, unsigned __int64 * const new_size, const int block_use, const char * const file_name, const int line_number, const bool reallocation_is_allowed) Line 595	C++
 	_realloc_dbg(void * block, unsigned __int64 requested_size, int block_use, const char * file_name, int line_number) Line 758	C++
 	realloc(void * block, unsigned __int64 size) Line 41	C++
 	ply::Heap_CRT::Operator::realloc(void * ptr, unsigned __int64 newSize) Line 26	C++
 	ply::ChunkCursor::toString(ply::ChunkCursor && start, const ply::ChunkCursor & end) Line 88	C++
 	ply::MemOutStream::moveToString() Line 171	C++
 	ply::PathFormat::joinAndNormalize(ply::ArrayView<ply::StringView const> components) Line 198	C++
 	ply::Path<1>::join<char const * &,char (&)[256]>(const char * & <pathComponentArgs_0>, char[256] & <pathComponentArgs_1>) Line 113	C++
 	main(int __formal, char * * __formal) Line 136	C++

Basically it looks like this block of code is doing a realloc:

if (!start.chunk->next && start.chunk->refCount == 1 && start.chunk->bytes == start.curByte) {
// Optimization: When there is only one chunk and only one reference to the chunk, we can
// reuse its memory, and no memcpy is performed:
u32 numBytes = start.chunk->writePos;
char* bytes = (char*) PLY_HEAP.realloc(start.curByte, numBytes);
String result = String::adopt(bytes, numBytes);
start.chunk.release();
start.curByte = nullptr;
return result;
}

on a pointer that was allocated via PLY_HEAP.allocAligned in ChunkListNode::allocate (from MemOutStream's constructor).

DLMalloc doesn't care, only the CRT heap does 😿

Not sure what's the best solution here, looks like doing a String::adopt from an aligned allocation is never going work with the CRT heap. But on the other hand if you just need 16 bytes alignment, perhaps you don't even need allocateAligned (that's already the min alignment for normal allocations with DLMalloc and CRT heap (in 64bits mode only though).

Hi Jeremy, good catch. I don't believe there was any reason for ChunkListNode to use allocAligned, so I just submitted a change to remove it. The test suite now passes when using Windows CRT malloc, so I imagine it should fix your problem too. Feel free to reopen otherwise.

(If there's ever a need to create ChunkListNodes with an alignment different from default malloc, we can always add a separate codepath for that in the future.)

Awesome, thanks!