Create a 3-D array with malloc but got RE and MLE

Question

I wrote a function with three parameter as height, width, and depth. After create a 3-D array with malloc, I return a unsigned*** as array’s name.

The other function is meant to delete this dynamic 3-D array. Using array’s name as parameter, and free it in the function.

But I got some RE and MLE in the submit, I wonder where shouldn’t I do?

unsigned ***new_3d_array(unsigned n, unsigned m, unsigned k)
{
    unsigned ***arr3D = (unsigned***)malloc(n * sizeof(unsigned**));
    int i, j;

    for (i = 0; i < n; i++)
    {
        arr3D[i] =  (unsigned**)malloc(m * sizeof(unsigned*));
        for (j = 0; j < m; j++)
        {
            arr3D[i][j] = (unsigned*)malloc(k * sizeof(unsigned));
        }
    }

    return arr3D;
}
void delete_3d_array(unsigned ***arr)
{
    int i = 0, j = 0;
    while (arr[i] != NULL)
    {
        j = 0;

        while (arr[i][j] != NULL)
        {
            free(arr[i][j]);
            j++;
        }
        free(arr[i]);
        i++;
    }
    free(arr);
}

Answer 1

In your delete_3d_array function, you rely on the first 'out-of-bounds' pointer elements of arr[i] and arr[i][j] being NULL - but this is not safe, unless you explicitly add these 'extra' values (sentinels) in your new_3d_array function and assign NULL to these:

unsigned ***new_3d_array(unsigned n, unsigned m, unsigned k)
{
    unsigned ***arr3D = malloc((n+1) * sizeof(unsigned**)); /// Add space for a NULL marker.
    unsigned i, j; /// As suggested by chux - Makes more sense to avoid mixing types.

    arr3D[n] = NULL; /// Explicitly set the 'end marker' to NULL
    for (i = 0; i < n; i++)
    {
        arr3D[i] = malloc((m+1) * sizeof(unsigned*)); /// Again, add end-marker...
        arr3D[i][m] = NULL; // ... and definitively set it to NULL!
        for (j = 0; j < m; j++)
        {
            arr3D[i][j] = malloc(k * sizeof(unsigned));
        }
    }

    return arr3D;
}

Also, see here on casting the return value of malloc.

EDIT/APPENDIX:
The above code will fix your RE problem, assuming that RE is "Run-Time Error" (that is, de-referencing an invalid/uninitialized pointer). However, there is still a problem caused by multiple calls to malloc (or calloc), known as "heap fragmentation." Each of your innermost loop's malloc calls will ask the operating system to 'look for' space to assign to the requested memory, and this can (especially if your m dimension is large) make the O/S 'think' it's run out of memory.

You can help prevent this from happening by calling malloc much less often (and for larger blocks), and then calculating (rather than allocating) your 2nd-level pointers:

unsigned*** new_3d_array(unsigned n, unsigned m, unsigned k)
{
    unsigned*** arr3D = malloc((n + 1) * sizeof(unsigned**)); // Add space for a NULL marker.
    unsigned i, j;
    arr3D[n] = NULL; /// Explicitly set the 'end marker' to NULL
    for (i = 0; i < n; i++) {
        arr3D[i] = malloc(m * sizeof(unsigned*));
        unsigned* block = malloc(sizeof(unsigned) * m * k); // Allocate one BIG block of m x k 
        for (j = 0; j < m; j++)
            arr3D[i][j] = block + j * m;// First element (j == 0) will be "block", others will be incremented by "m"
    }
    return arr3D;
}

Of course, you would then need to modify your delete_3d_array function accordingly:

void delete_3d_array(unsigned*** arr)
{
    int i = 0;
    while (arr[i] != NULL) {
        free(arr[i][0]); // This will free the WHOLE BLOCK (m x k) allocated above!
        free(arr[i]);
        i++;
    }
    free(arr);
}

You could, in theory, even change the 'outer' loop in a similar way; but then, the syntax for calculating pointer addresses for the sub-arrays becomes quite arcane.

Feel free to ask for any further clarification and/or explanation.

Answer 2

I believe better to use calloc, instead of malloc. Easier interface, and reduces the possibility of incorrect initialization.

Style note: better to use for loops, instead of while loops to free the data.

unsigned ***new_3d_array(unsigned n, unsigned m, unsigned k)
{
    unsigned ***arr3D = calloc(n+1, sizeof(*arr3d));
    for (int i = 0; i < n; i++)
    {
        arr3D[i] =  calloc(m+1, sizeof(*arr3D[i]));
        for (int j = 0; j < m; j++)
        {
            arr3D[i][j] = calloc(k, sizeof(*arr3D[i][j]));
        }
    }

    return arr3D;
}
void delete_3d_array(unsigned ***arr)
{
    for (int i=0 ; arr[i] != NULL ; i++ ) {
        for (int j = 0; arr[i][j] != NULL ; j++) {
            free(arr[i][j]);
        }
        free(arr[i]);
        arr[i] = NULL ;
    }
    free(arr);
}

Answer 3

@adrian has given one possible solution.

There is another way by passing the size of the array to the delete function. You only need to pass n and m to the function.

void delete_3d_array(unsigned ***arr, int n, int m)
{
    int i,j;
    for (i=0; i<n; i++)
    {
        for (j=0; j<m; j++)
        {
            free(arr[i][j]);
        }
        free(arr[i]);
    }
    free(arr);
}