Can you explain where and how the code for freeing the dynamically allocated memory in the code below is wrong?

Question

This is the code to initialize, print, and release a two-dimensional array through memory dynamic allocation.

#define  _CRT_SECURE_NO_WARNINGS
#include <stdio.h>
#include <stdlib.h>

int main() {
    int idx, row, col, x, y;

    int **pptr = (int **)malloc(sizeof(int *) * 8);

    if (NULL == pptr) {
        printf("ERROR: malloc failed.\n");
        exit(1);
    }

    for (idx = 0; idx < 8; idx++) {
        *(pptr + idx) = (int*)malloc(sizeof(int) * 6);

        if (NULL == *(pptr + idx)) {
            printf("ERROR: malloc failed.\n");
            exit(1);
        }
    }

    for (row = 0; row < 8; row++) {
        for (col = 0; col < 6; col++) {
            *(*(pptr + row) + col) = 6 * row + col;
        }
    }

    for (row = 0; row < 8; row++) {
        for (col = 0; col < 6; col++) {
            printf("%3d", *(*(pptr + row) + col));
        }
        printf("\n");
    }

    for (idx = 0; idx < 8; idx++) {
        free(*(pptr + idx));
        if (NULL != *(pptr + idx)) {
            *(pptr + idx) = NULL;
        }
    }

    free(pptr);
    if (NULL != pptr) {
        pptr = NULL;
    }

    return 0;
}

If something is wrong, how can I fix it?

Why are you torturing yourself with the unreadable `*(foo+bar)` notation? Use array syntax. You'll likely fix your bug in the process. — Mat, Sep 04 '21 at 11:47
While it's not exactly *wrong*, the last `if` is useless. You might just do `pptr = NULL` unconditionally, but that too is useless, as you return just after that. Likewise, the similar `if` in the `for` loop above is useless. — , Sep 04 '21 at 12:00
You're printing all your error messages to the wrong stream. Output belongs on stdout, error messages belong on stderr. — William Pursell, Sep 04 '21 at 12:13
Don't cast the value returned by malloc: https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc — William Pursell, Sep 04 '21 at 12:17

chqrlie · Answer 1 · 2021-09-04T12:31:24.467

The code posted is OK, but note these remarks:

it is somewhat difficult to read: using pptr[idx] instead of *(pptr + idx) would improve readability without consequences as both syntaxes are equivalent.
it has redundant tests: There is no need to test if a pointer is not NULL before your set it to NULL.
casting the return value of malloc() is useless in C (albeit it is compulsory in C++ where you should not be using malloc() anyway).
variables x and y are unused.
you do not free allocated memory in case of error, which is not really an error as the OS will reclaim all of the process' memory upon exit, which you request immediately. Note that you could write return 1; in place of exit(1) with the same effect from the main() function.
defining the number of columns and rows in the matrix would help enforce consistency and improve readability.
you could use row instead of idx in the allocation and deallocation loops.

Here is a modified version:

#define  _CRT_SECURE_NO_WARNINGS
#include <stdio.h>
#include <stdlib.h>

#define NROWS  8
#define NCOLS  6

int main() {
    int row, col;
    int **pptr = malloc(sizeof(*pptr) * ROWS);

    if (!pptr) {
        fprintf(stderr, "ERROR: malloc failed.\n");
        exit(1);
    }

    for (row = 0; row < NROWS; row++) {
        pptr[row] = malloc(sizeof(*pptr[row]) * NCOLS);
        if (!pptr[row]) {
            fprintf(stderr, "ERROR: malloc failed.\n");
            exit(1);
        }
    }

    for (row = 0; row < NROWS; row++) {
        for (col = 0; col < NCOLS; col++) {
            pptr[row][col] = NCOLS * row + col;
        }
    }

    for (row = 0; row < NROWS; row++) {
        for (col = 0; col < NCOLS; col++) {
            printf("%3d", pptr[row][col]);
        }
        printf("\n");
    }

    for (row = 0; row < NROWS; row++) {
        free(pptr[row]);
        pptr[row] = NULL;
    }

    free(pptr);
    pptr = NULL;

    return 0;
}

You are not allocating a real 2D matrix. Here is an alternative approach:

#define  _CRT_SECURE_NO_WARNINGS
#include <stdio.h>
#include <stdlib.h>

#define NROWS  8
#define NCOLS  6

int main() {
    int row, col;
    /* allocate a 2D matrix: an array or arrays of NCOLS ints */
    int (*pptr)[NCOLS] = malloc(sizeof(*pptr) * ROWS);

    if (!pptr) {
        fprintf(stderr, "ERROR: malloc failed.\n");
        exit(1);
    }

    for (row = 0; row < NROWS; row++) {
        for (col = 0; col < NCOLS; col++) {
            pptr[row][col] = NCOLS * row + col;
        }
    }

    for (row = 0; row < NROWS; row++) {
        for (col = 0; col < NCOLS; col++) {
            printf("%3d", pptr[row][col]);
        }
        printf("\n");
    }

    free(pptr);
    pptr = NULL;

    return 0;
}

@neuezeal: you can accept this answer by clicking on the grey checkmark below its score. — chqrlie, Sep 05 '21 at 11:06

Can you explain where and how the code for freeing the dynamically allocated memory in the code below is wrong?

1 Answers1