1

I have a function which builds an array of strings (char*) .
After I finished using the array I want to free its memory but then I get a CrtIsValidHeapPointer Error.

Code:

int main(int argc, char * argv[])
{
    char** arr = NULL;

    creating_array(&arr);    // Building the array

    //free each string memory
    for (size_t i = 0; i < 4; i++)
    {
        free(arr[i]);
    }

    // until here everything works fine :)

    //free the array memory    
    free(arr);        // Error CrtIsValidHeapPointer

    return 0;
}

void creating_array(char*** pArr)
{
    char** arr = (char**)malloc(4);
    arr[0] = (char*)malloc(5 * sizeof(char));
    strcpy(arr[0], "aaaa");
    arr[1] = (char*)malloc(5 * sizeof(char));
    strcpy(arr[1], "bbbb");
    arr[2] = (char*)malloc(5 * sizeof(char));
    strcpy(arr[2], "cccc");
    arr[3] = (char*)malloc(5 * sizeof(char));
    strcpy(arr[3], "dddd");

    *pArr = arr;
}

Why does it happen?

AlonP
  • 903
  • 1
  • 9
  • 16
  • You don't allocate memory correctly. – ameyCU Oct 05 '15 at 17:00
  • What is the hard-coded `malloc(4)` intended for? – Weather Vane Oct 05 '15 at 17:03
  • You want `char** arr = malloc(4 * sizeof(char*));`. Also, [don't cast `malloc`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). @ameyCU fixed. – MooseBoys Oct 05 '15 at 17:05
  • You right. the problem was that I tought that a 4 slots array of char** is in size of 4 bytes but the correct way is to tell him to molloc `(4 * sizeof(char**))`. I can assume that char** pointer needs to save more information so its size is not only one byte. @MooseBoys – AlonP Oct 10 '15 at 12:59

2 Answers2

2

Please try, in creating_array

char **arr ;
arr=(char **) calloc( 4 , sizeof(char *));
BobRun
  • 756
  • 5
  • 12
  • Thanks, you are right the explanation is necessary. – BobRun Oct 05 '15 at 17:32
  • `calloc()` has the advantage of automatically zeroing memory, which can save you a step. `malloc()` can return memory with garbage in it. – clearlight Oct 05 '15 at 17:58
  • You absolutely right! the problem was that I tought that a 4 slots array of char** is in size of 4 bytes so I wrote molloc(4) but the correct way is to tell him to molloc (4 * sizeof(char**)). I can assume that char** pointer needs to save more information so its size is not only one byte. Thank you man :) – AlonP Oct 10 '15 at 13:02
1

Here is the working code. It compiles and runs now.

Generally after freeing a pointer, people set it to NULL to avoid confusion (so that any time a pointer is not null it points to valid memory). That avoids bugs. Also it is legal to free(NULL), so you don't get into very severe and hard-to- debug problems that happen if you double-free an address.

One important point, is that in this case, the parenthesis in this case (*pArr)[2] are important, to override the operator precedence in C. If you try *pArr[2] it assumes you mean to de-reference the pointer stored at element [2]. (*pArr)[2] means return element at element to from the the location at the address pointed to by pArr. The reason C assumes the other case and that you need parens in this case is that the other use is much much more common, so it is convenient.

Note: ALWAYS check return values for malloc() and function calls and have a strategy to catch and log errors. Otherwise as you start writing bigger programs you will find them extremely difficult, troublesome or nearly impossible to debug.

Another thing is to create named constants instead of literals, because then it is clear what the number is and how it is used, and if that number is needed in more than one place it can be changed in one place. It makes the program easier to read and understand.

#include <stdio.h>
#include <string.h>
#include <strings.h>
#include <stdlib.h>

#define STRING_COUNT 4

char **create_array(char ***pArr);

int 
main()
{
    char **arr = NULL;

    if (create_array(&arr) == NULL) {
        fprintf(stderr, "out of memory - exiting\n");
        return -1;
    }
    for (size_t i = 0; i < STRING_COUNT; i++) {
        printf("%s\n", arr[i]);
    }
    for (size_t i = 0; i < STRING_COUNT; i++) {
        free(arr[i]);
        arr[i] = NULL;
    }
    free(arr);      
    return 0;
}

char **
create_array(char ***pArr)
{
    if ((*pArr = malloc(STRING_COUNT * sizeof (char **))) == NULL) {
       return NULL;
    }
    if (((*pArr)[0] = strdup("aaaa")) == NULL) {
        return NULL;
    }
    if (((*pArr)[1] = strdup("bbbb")) == NULL) {
        free((*pArr)[0]);
        *pArr[0] = NULL;
        return NULL;
    }
    if (((*pArr)[2] = strdup("bbbb")) == NULL) {
        free((*pArr)[0]);
        *pArr[0] = NULL;
        free((*pArr)[1]);
        *pArr[1] = NULL;
        return NULL;
    }
    if (((*pArr)[3]= strdup("bbbb")) == NULL) {
        free((*pArr)[0]);
        *pArr[0] = NULL;
        free((*pArr)[2]);
        *pArr[1] = NULL;
        free((*pArr)[2]);
        *pArr[2] = NULL;
        return NULL;
    }
    return *pArr;

}
clearlight
  • 12,255
  • 11
  • 57
  • 75