Search code examples
cmemory-managementmallocrealloc

ERROR "realloc(): invalid next size" when allocating memory to const char*** variable


I have a function

populateAvailableExtensions(const char** gAvailableExtensions[], int gCounter)

which take a pointer to an array of strings and the number of elements in the array as parameters.

I allocate initial memory to that array using malloc(0). Specs say that it will either return a null pointer or a unique pointer that can be passed to free().

  int currentAvailableExtensionCount = gCounter;

This variable will store number of string in gAvailableExtensions.

Inside this for loop

for (int i = 0; i < availableExtensionCount; ++i)

I have this piece of code

    size_t sizeOfAvailableExtensionName =
        sizeof(availableExtensionProperties[i].name);

    reallocStatus = realloc(*gAvailableExtensions, sizeOfAvailableExtensionName);

    memcpy(&(*gAvailableExtensions)[currentAvailableExtensionCount],
           &availableExtensionProperties[i].name,
           sizeOfAvailableExtensionName);

    ++currentAvailableExtensionCount;

where

availableExtensionProperties[i].name

returns a string.

This is how that struct is defined

typedef struct Stuff {
    char        name[MAX_POSSIBLE_NAME];
    ...
    ...
} Stuff;

realloc(*gAvailableExtensions, sizeOfAvailableExtensionName);

should add memory of size sizeOfAvailableExtensionName to *gAvailableExtensions de-referenced array.

memcpy(&(*gAvailableExtensions)[currentAvailableExtensionCount],
           &availableExtensionProperties[i].name,
           sizeOfAvailableExtensionName);

should copy the string (this sizeOfAvailableExtensionName much memory) from

&availableExtensionPropterties[i].name

address to

&(*gAvailableExtensions)[currentAvailableExtensionCount]

address.


But I don't think the code does what I think it should because I'm getting this error

realloc(): invalid next size
Aborted
(core dumped) ./Executable

EDIT: Full code

uint32_t populateAvailableExtensions(const char** gAvailableExtensions[], int gCounter) {

  int currentAvailableExtensionCount = gCounter;

  void* reallocStatus;

  uint32_t availableExtensionCount = 0;

  vkEnumerateInstanceExtensionProperties(
      VK_NULL_HANDLE, &availableExtensionCount, VK_NULL_HANDLE);

  VkExtensionProperties availableExtensionProperties[availableExtensionCount];

  vkEnumerateInstanceExtensionProperties(
      VK_NULL_HANDLE, &availableExtensionCount, availableExtensionProperties);

  for (int i = 0; i < availableExtensionCount; ++i) {

    size_t sizeOfAvailableExtensionName =
        sizeof(availableExtensionProperties[i].extensionName);

    reallocStatus = realloc(*gAvailableExtensions, sizeOfAvailableExtensionName);

    memcpy(&(*gAvailableExtensions)[currentAvailableExtensionCount],
           availableExtensionProperties[i].extensionName,
           sizeOfAvailableExtensionName);

    ++currentAvailableExtensionCount;
  }

  return currentAvailableExtensionCount;
}

This is how an external function calls on that one,

  uint32_t availableExtensionCount = 0;
  availableExtensions              = malloc(0);
  availableExtensionCount          = populateAvailableExtensions(&availableExtensions);

and

const char** availableExtensions;

is declared in header file.

EDIT 2: Updated the code, now gCounter holds the number of elements in gAvailableExtensions


Solution

  • This loop is totally messy:

    for (int i = 0; i < availableExtensionCount; ++i) {
    
        size_t sizeOfAvailableExtensionName =
            sizeof(availableExtensionProperties[i].extensionName);
    
        reallocStatus = realloc(*gAvailableExtensions, sizeOfAvailableExtensionName);
    
        memcpy(&(*gAvailableExtensions)[currentAvailableExtensionCount],
               availableExtensionProperties[i].extensionName,
               sizeOfAvailableExtensionName);
    
        ++currentAvailableExtensionCount;
      }
    

    I assume the only lines that does what you expect them to do, are the lines for (int i = 0; i < availableExtensionCount; ++i) and ++currentAvailableExtensionCount;

    First, the typical way to use realloc is like this:

    foo *new_p = realloc(p, new_size);
    if (!new_p)
       handle_error();
    else
       p = new_p;
    

    The point is that realloc will not update the value of p if a reallocation happens. It is your duty to update 'p'. In your case you never update *gAvailableExtensions. I also suspect that you don't calculate sizeOfAvailableExtensionCount correctly. The operator sizeof always return a compile time constant, so the realloc doesn't actuall make any sense.

    The memcpy doesn't actally make any sense either, since you are copying the string into the memory of a pointer array (probably with an additional buffer overflow).

    You said that *gAvailableExtensions is a pointer to an array of pointers to strings. That means that you have to realloc the buffer to hold the correct number of pointers, and malloc memory for each string you want to store.

    For this example, I assume that .extensionName is of type char * or char[XXX]:

    // Calculate new size of pointer array
    // TODO: Check for overflow
    size_t new_array_size = 
      (currentAvailableExtensionCount + availableExtensionCount) * sizeof(*gAvailableExtensions);
    
    char **tmp_ptr = realloc(*gAvailableExtensions, new_array_size);
    if (!tmp_ptr)
        {
           //TODO: Handle error;
           return currentAvailableExtensionCount;
        } 
    *gAvailableExtensions = tmp_ptr;
    
    // Add strings to array
    for (int i = 0; i < availableExtensionCount; ++i) 
      {
        size_t length = strlen(availableExtensionProperties[i].extensionName);
    
        // Allocate space for new string
        char *new_s = malloc(length + 1); 
        if (!new_s)
           { 
             //TODO: Handle error;
             return currentAvailableExtensionCount;
           }
    
        // Copy string
        memcpy (new_s, availableExtensionProperties[i].extensionName, length + 1);
    
        // Insert string in array
        (*gAvailableExtensions)[currentAvailableExtensionCount] = new_s;
    
        ++currentAvailableExtensionCount;
      }
    

    If you can guarantee that the lifetime of availableExtensionProperties[i].extensionName is longer than *gAvailableExtensions, you can simplify this a little bit by dropping malloc and memcpy in the loop, and do:

    char *new_s  = availableExtensionProperties[i].extensionName;
    (*gAvailableExtensions)[currentAvailableExtensionCount] = new_s;
    

    Some harsh words at the end: It seems like you have the "Infinite number of Monkeys" approach to programming, just hitting the keyboard until it works.

    Such programs will just only give the illusion of working. They will break in spectacular ways sooner or later.

    Programming is not a guessing game. You have to understand every piece of code you write before you move to the next one.