How do I correctly free my arrays without errors?

I came up with this code with the help of @GWW and now I can't free a char**.

Here's my code (it just reads an input file and prints the names in it on the screen):

    /*   deallocate2D
corresponding function to dynamically deallocate 2-dimensional array using
 * malloc.
 * accepts a char** as the "array" to be allocated, and the number of rows.
 * as with all dynamic memory allocation, failure to free malloc'ed memory
 * will result in memory leaks
 */
void deallocate2D(char** array, int nrows) {

    /*  deallocate each row  */
    int i;
    for (i = 0; i < nrows; i++) {
        free(array[i]);
    }

    /*  deallocate array of pointers  */
    free(array);
}

int readInputFile(FILE *fp, char **file_images) {
    num_lines = 0;
    int s = 10;
    char line[MAX_LENGTH];
    char **final_filenames;

    while (fgets(line, sizeof line, fp) != NULL) /* read a line */ {
        if (line[0] != '\n') {
            if (num_lines >= s) {
                s += 100;
                if ((file_images = (char**) realloc(file_images, s * sizeof (char*))) == NULL) {
                    printf("Error reallocating space for 2d array: %s\n", strerror(errno));
                    return -1;
                }
            }
            if ((file_images[num_lines] = malloc(MAX_LENGTH * sizeof (char))) == NULL) {
                printf("Error allocating space for 2d array: %s\n", strerror(errno));
                return -1;
            }

            strncpy(file_images[num_lines], line, MAX_LENGTH);
            if (file_images[num_lines] == NULL) {
                printf("Strncpy failed: %s\n", strerror(errno));
                return -1;
            }
            printf("name of file %d is: %s \n", num_lines, file_images[num_lines]);
            num_lines++;
        }
    }
    printf("Num_lines: %d\n",num_lines);
    //realloc to number of lines in the file, to avoid wasting memory
    if开发者_开发技巧 ((final_filenames = realloc(file_images, num_lines * sizeof (char*))) == NULL) {
        printf("Error reallocating space for 2d array: %s\n", strerror(errno));
        return -1;
    } else {
        file_images = final_filenames;
        deallocate2D(final_filenames, num_lines);
    }
    return 0;
    //don't forget to free lines 2d array! (here or at the end of the code)
}

int main(int argc, char *argv[]) {
    //pixel* image;
    char **images_filenames;

    //check parameters
    if (argc < 4) {
        printf("Incorrect usage.\nPlease use \"./invert input_filename.ppm charWidth charHeight \"\n");
        return -1;
    }

    printf("Opening input file [%s]\n", argv[1]);
    FILE *fpin = fopen(argv[1], "r");
    if (fpin == NULL) {
        printf("Could not open input file\n");
        return -1;
    }
    if ((images_filenames = ((char**) malloc(10 * sizeof (char*)))) == NULL) {
        printf("Error allocating initial space for 2d array: %s\n", strerror(errno));
        return -1;
    }

    if (readInputFile(fpin, images_filenames) == -1) {
        printf("Error reading image filenames from input\n");
        return -1;
    }

    fclose(fpin);
    printf("###########\n");

    deallocate2D(images_filenames, num_lines);

    printf("Done!\n");
    return 0;
}

So, I don't understand why can't I free final_filenames and images_filenames.

The error that this code gives me is:

*** glibc detected *** ./main: double free or corruption (!prev): 0x0986d228 ***

How do I correctly free my arrays without errors?

The problems are that you are freeing a pointer that may already have been freed, and you don't know how much space is in use don't have a pointer to the most recently allocated space (in general) so you can't free the memory accurately. In main(), you have:

char **images_filenames;                     

[...]                                

if ((images_filenames = ((char**) malloc(10 * sizeof (char*)))) == NULL) {

[...]

if (readInputFile(fpin, images_filenames) == -1) {

[...]

deallocate2D(images_filenames, num_lines);

You allocate 10 character pointers, and then pass that array to the readInputFile() function. Inside that function, there is code to reallocate the array, but you have not provided a way for the main program to know what that new address is. The way you do that is either by passing a pointer to whatever it is you want modified, or you have the function return the modified value (or you resort to sordid practices like using global variables instead of parameters - but you shouldn't do that).

So, you need:

if (readInputFile(fpin, &images_filenames) == -1) {

And in the readInputFile() function, you need a whole lot of changes - the big one to deal with the triple-pointer argument, and then a variety of coding problems:

int readInputFile(FILE *fp, char ***ppp_files)
{
    num_lines = 0;
    int s = 10;
    char line[MAX_LENGTH];
    char **file_images = *ppp_files;
    char **final_filenames;

Update: I didn't notice that this was simply initializing num_lines, not declaring it. Thus, num_lines must be a global variable of some sort...some of the commentary below needs adjusting to allow for this.

So far, the change is (almost) trivial; we're getting a pointer to a 'char **', hence the triple pointer argument. To simplify the following code, make a local copy of the parameter under the old name (file_images) and initialized it with the value that the argument points at. Following code can then continue to work with file_images; just ensure that the argument is updated before returning.

Except...

You assume that 's = 10', but really, you should have the main function tell you how many rows were available. It did allocate 10 rows, but it is not clear without careful scrutiny that was the case. You should have the main() program say how many rows were preallocated - an extra argument to the function. You also face the problem that the main() program cannot tell the deallocate2D() function how many rows are in the array because it does not know. It is not clear how your code compiles; you have a local variable num_lines here, but there's a reference to a variable num_lines in main() for which there is no declaration. The local variable masks any global variable.

    while (fgets(line, sizeof line, fp) != NULL) {
        if (line[0] != '\n') {
            if (num_lines >= s) {
                s += 100;

Adding a large number of rows is a good idea; it 'amortizes' the cost of the reallocations.

               if ((file_images = (char**) realloc(file_images, s * sizeof (char*))) == NULL)

There are specific problems with the technique you've used though. Pure code style: when a line contains an if with an embedded assignment and it gets too long, split the assignment out before the condition:

                file_images = (char**) realloc(file_images, s * sizeof (char*));
                if (file_images == NULL)

Now there's just a subtle bug left. What happens if realloc() fails...

That's right, you've leaked the memory because the value in file_images is null, so there is no way to release what it used to point to. Never write:

x = realloc(x, size);

It leaks memory on failure! Hence, you need:

                char **new_space = realloc(file_images, s * sizeof (char*));
                if (new_space == NULL)
                {
                    printf("Error reallocating space for 2d array: %s\n",
                           strerror(errno));
                    *ppp_files = file_images;
                    return -1;
                }
            }

As a general rule, error messages should be printed on stderr; I haven't fixed this.

Note that I carefully copied back the last (non-null) value of file_images into the variable in the main program. It might be appropriate to do the same with the size, too (another interface change), or use a structure to encapsulate the array - size and pointer to its base.

        if ((file_images[num_lines] = malloc(MAX_LENGTH * sizeof (char))) == NULL)
        {
             printf("Error allocating space for 2d array: %s\n", strerror(errno));
             return -1;              
        }

This error return needs to set *ppp_files = file_images;.

            strncpy(file_images[num_lines], line, MAX_LENGTH);


            if (file_images[num_lines] == NULL) {               
                printf("Strncpy failed: %s\n", strerror(errno));
                return -1;
            }                                                                   

This test is odd; you know that file_images[num_lines] is not null, and strncpy() doesn't change that. You don't need the test and error handling.

            printf("name of file %d is: %s \n", num_lines, file_images[num_lines]);
            num_lines++;
        }                                                              
    }
    printf("Num_lines: %d\n",num_lines);

OK...

    //realloc to number of lines in the file, to avoid wasting memory

Nice touch. It is barely worth it; even on a 64-bit machine, you are wasting less than 1 KiB at most. However, no harm in being tidy - good.

    if ((final_filenames = realloc(file_images, num_lines * sizeof (char*))) == NULL) {
        printf("Error reallocating space for 2d array: %s\n", strerror(errno));
        return -1;

Again, you need to set *ppp_files = file_images; before return.

    } else {
        file_images = final_filenames;

This does not affect the value in the main() program. It would need to be *ppp_files = file_images; again.

        deallocate2D(final_filenames, num_lines);

Hold on - you deallocate all your carefully allocated space? So you aren't going to use it after all? The assignment above merely copied a pointer value around; it did not make a copy of the memory...

    }
    return 0;
    //don't forget to free lines 2d array! (here or at the end of the code)
}

This comment is wrong - on successful return, the memory is already deallocated.

Lemme guess - you don't use 'vim' or another 'vi' derivative for editing. People who do have the opening brace of their functions in column 1, because then you can jump forwards or backwards through the file to the start of the next or previous function using ']]' or '[['. It is irksome working with code where that does not work.

Well, that's a starting diagnosis... Here's working code using a structure to relay the array of file names around. I've left the body of the readInputFile() function using local variables that are copied out of the structure, and ensured that the structure is properly updated at all times.

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

enum { MAX_LENGTH = 512 };

typedef struct FileNameArray
{
    size_t   nfiles;    /* Number of file names allocated and in use */
    size_t   maxfiles;  /* Number of entries allocated in array */
    char   **files;     /* Array of file names */
} FileNameArray;

static void deallocate2D(FileNameArray *names)
{
    for (size_t i = 0; i < names->nfiles; i++)
        free(names->files[i]);
    free(names->files);
    names->nfiles   = 0;
    names->files    = 0;
    names->maxfiles = 0;
}

static int readInputFile(FILE *fp, FileNameArray *names)
{
    int    num_lines  = names->nfiles;
    int    max_lines  = names->maxfiles;
    char **file_names = names->files;
    char line[MAX_LENGTH];
    char **final_filenames;

    while (fgets(line, sizeof line, fp) != NULL)
    {
        if (line[0] != '\n')
        {
            /* Remove newline from end of file name */
            char *nl = strchr(line, '\n');
            if (nl != 0)
                *nl = '\0';
            if (num_lines >= max_lines)
            {
                max_lines += 100;
                char **space = realloc(file_names, max_lines * sizeof (char*));
                if (space == NULL)
                {
                    fprintf(stderr, "Error reallocating space for 2d array: %s\n",
                            strerror(errno));
                    return -1;
                }
                names->maxfiles = max_lines;
                names->files = space;
                file_names = space;
            }
            if ((file_names[num_lines] = malloc(strlen(line) + 1)) == NULL)
            {
                fprintf(stderr, "Error allocating space for 2d array: %s\n",
                        strerror(errno));
                return -1;
            }
            names->nfiles++;
            strcpy(file_names[num_lines], line);
            printf("name of file %d is: %s \n", num_lines, file_names[num_lines]);
            num_lines++;
        }
    }

    printf("Num_lines: %d\n", num_lines);
    //realloc to number of lines in the file, to avoid wasting memory
    if ((final_filenames = realloc(file_names, num_lines * sizeof (char*))) == NULL)
    {
        fprintf(stderr, "Error reallocating space for 2d array: %s\n",
                strerror(errno));
        return -1;
    }
    names->maxfiles = num_lines;
    names->files    = final_filenames;
    return 0;
}

int main(int argc, char *argv[])
{
    FileNameArray names = { 0, 0, 0 };

    //check parameters
    if (argc < 4)
    {
        fprintf(stderr, "Usage: %s input_filename.ppm charWidth charHeight\n",
                argv[0]);
        return -1;
    }

    printf("Opening input file [%s]\n", argv[1]);
    FILE *fpin = fopen(argv[1], "r");
    if (fpin == NULL) {
        fprintf(stderr, "Could not open input file %s (%s)\n",
                argv[1], strerror(errno));
        return -1;
    }

    if ((names.files = malloc(10 * sizeof (char*))) == NULL)
    {
        fprintf(stderr, "Error allocating initial space for 2d array: %s\n",
                strerror(errno));
        return -1;
    }
    names.maxfiles = 10;

    if (readInputFile(fpin, &names) == -1)
    {
        fprintf(stderr, "Error reading image filenames from input\n");
        return -1;
    }

    fclose(fpin);
    printf("###########\n");

    deallocate2D(&names);

    printf("Done!\n");
    return 0;
}