I'm currently building a node.js addon for libcurl. Right now I'm trying to correctly use v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory to update v8 on the amount of memory being allocated by libcurl, and to do that I need to wrap the above mentioned functions that libcurl uses, using curl_global_init_mem().

I saw the following code being used here that basically does the same thing, but for another lib.

I currently have the following code (almost identical to the one above):

struct MemWrapper {
    size_t size;
    double data;
};

#define MEMWRAPPER_SIZE offsetof( MemWrapper, data )

inline void* MemWrapperToClient( MemWrapper* memWrapper ) {
    return static_cast<void*>( reinterpret_cast<char*>( memWrapper ) + MEMWRAPPER_SIZE );
}

inline MemWrapper* ClientToMemWrapper( void* client ) {
    return reinterpret_cast<MemWrapper*>( static_cast<char*>( client ) - MEMWRAPPER_SIZE );
}

void AdjustMem( ssize_t diff )
{
    Nan::AdjustExternalMemory( static_cast<int>( diff ) );
}

void* MallocCallback( size_t size )
{
    size_t totalSize = size + MEMWRAPPER_SIZE;
    MemWrapper* mem = static_cast<MemWrapper*>( malloc( totalSize ) );
    if ( !mem ) return NULL;
    mem->size = size;
    AdjustMem( totalSize );
    return MemWrapperToClient( mem );
}

void FreeCallback( void* p )
{
    if ( !p ) return;
    MemWrapper* mem = ClientToMemWrapper( p );
    ssize_t totalSize = mem->size + MEMWRAPPER_SIZE;
    AdjustMem( -totalSize );
    free( mem );
}

void* ReallocCallback( void* ptr, size_t size )
{
    if ( !ptr ) return MallocCallback( size );
    MemWrapper* mem1 = ClientToMemWrapper( ptr );
    ssize_t oldSize = mem1->size;
    MemWrapper* mem2 = static_cast<MemWrapper*>( realloc( mem1, size + MEMWRAPPER_SIZE ) );
    if ( !mem2 ) return NULL;
    mem2->size = size;
    AdjustMem( ssize_t( size ) - oldSize );
    return MemWrapperToClient( mem2 );
}

char* StrdupCallback( const char* str )
{
    size_t size = strlen( str ) + 1;
    char* res = static_cast<char*>( MallocCallback( size ) );
    if ( res ) memcpy( res, str, size );
    return res;
}

void* CallocCallback( size_t nmemb, size_t size )
{
    size_t totalSize = size + MEMWRAPPER_SIZE;
    MemWrapper* mem = static_cast<MemWrapper*>( calloc( nmemb, totalSize ) );
    if ( !mem ) return NULL;
    mem->size = nmemb * size;
    AdjustMem( nmemb * totalSize );
    return MemWrapperToClient( mem );
}

I hook it up with libcurl calling:

curl_global_init_mem( CURL_GLOBAL_ALL, MallocCallback, FreeCallback, ReallocCallback, StrdupCallback, CallocCallback );

I compiled the add-on, and from what I can see, it's working fine, but as I'm unexperienced with C++, I need to know if this is correct. What kind of bad things can happen from modifying the pointer returned by calloc, malloc and prealloc like that? Is there any way to improve it?

  • 2
    One word: Alignment. Is ptr + MEMWRAPPER_SIZE correctly aligned for objects of size size? – Martin York May 17 '16 at 15:00
up vote 1 down vote accepted

I'm the author of the code you mentioned, with some suggestions by Nathan Zadoks. I've used the same idea in another project. So any mistakes others may find in the copied code will likely affect my code as well.

async?

One critical question is whether you intend to perform asynchroneous operations. If so, this code will likely fail because Nan::AdjustExternalMemory has to be called from the main Node thread, i.e. must not be called from a worker thread.

calloc

The CallocCallback is wasteful. Your code allocates

nmemb * (size + MEMWRAPPER_SIZE)

while a better implementation would only allocate

(nmemb * size) + MEMWRAPPER_SIZE

You can't get that by delegating to calloc, though. Instead I'd build on MallocCallback like this (similar to how StrdupCallback works, too):

void* CallocCallback( size_t nmemb, size_t size )
{
    void* ptr = MallocCallback( nmemb * size );
    if (!ptr) return NULL;
    memset( ptr, 0, nmemb * size ); // zero-fill
    return ptr;
}

alignment

Since Loki raised concerns regarding alignment, I'll explain my rationale.

Some platforms will suffer a performance penalty if certain data types are not aligned with memory addresses in a certain way, others may even prevent access, or pervent access for certain kins of operation (like SSE). I'm guessing that the largest data type affected by this in the library in question is probably double. So I'm using a double item as the data entry of the structure, and you copied that. If the compiler thinks alignment is important, it will lay out the struct in such a way that the data item is properly aligned if and only if the structure as a whole is properly aligned. It can accomplish this by introducing some padding between size and data.

So for example if sizeof(size_t) == 4 but sizeof(double) == 8 and more importantly alignof(double) == 8, then you'd get sizeof(MemWrapper) == 16, layed out as 4 bytes for size, 4 bytes padding, 8 bytes for data. You'd have offsetof(MemWrapper, data) == 8, taking both size and padding into account. The common calls like malloc will return memory which is properly aligned for any object type. So the wrapper will be aligned, and hence its data portion will be aligned as well.

The correct way to actually follow the malloc spec by the letter (instead of relying on some idea of what objects the library actually uses) would be using std::max_align_t (introduced in C++11) instead of double. But the difference becomes only important if the library is dealing in objects larger than a simple double. It might also be the case that the compiler doesn't pad the data structure by default, risking a performance impact or failure of some (e.g. SSE) operations. On GCC-like compilers the aligned attribute can be used to control alignment and prevent packing. A more portable way to achieve the same would probably be the use of an (anonymous) union between the size field and a std::max_align_t element. But I'd only do this if the application in question were using SSE or something like that, since on arhictectures where unaligned access is a more common problem I'd expect the compiler to take care of this.

  • About async, the wrappers are going to be added to libcurl only if it's built without the threaded resolver (the only thing that makes libcurl create another thread and call those functions from there). The addon itself doesn't call any of those functions. And thank you for the alignment insight! – JCM May 17 '16 at 22:00
  1. Check alignment.

I don;t believe you are correctly aligning your objects.

2. Realloc is wrong.

if realloc returns NULL the original pointer is not deallocated.

MemWrapper* mem2 = static_cast<MemWrapper*>( realloc( mem1, size + MEMWRAPPER_SIZE ) );
if ( !mem2 ) return NULL;

Here if realloc fails you have just leaked memory (as mem1 is still valid).

The standard usage pattern for realloc is:

 void* data = malloc(size);
 ...

 void* extra = realloc(data, newSize);
 if (extra) {
     data = extra;   // only reset data if the realloc worked.

 }

  • Thanks for the answer! Can you elaborate more on correctly aligning your objects.? – JCM May 17 '16 at 20:34
  • I don't follow your concerns about realloc. The way I see it, the semantics are delegated cleanly: if the inner realloc call returns NULL, then the outer ReallocCallback should return NULL as well. It's caller supposedly still holds the pointer it passed in, and can free that later on just as it would after a call to realloc. – MvG May 17 '16 at 21:44
  • @MvG: Yep that would make sense. – Martin York May 18 '16 at 5:40

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.