malloc & free in C with multidimensional arrays in C; Whats wrong with the code

Question

I am a beginner and i am working on a code where the multidim arrays are allocated with the malloc, malloc variant. I have to add an Array with a higher dimension (3D instead of 2D).

I discovered a strange behavior and i would be thankful for an explanation.

I know this is not really the proper way to handle multidimensional arrays. But it is already used in the program that i am modifying.

Thats a working code i used to isolate the problem in onlinegbd.com:

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

double ***AllocArray3D(short ix, int iy, int iz){
    double ***A3D; ix++; iy++;
    A3D = (double ***)malloc(ix * sizeof(double **));
    int i, j;
    for (i = 0; i < ix; i++) {
        A3D[i] = (double **)malloc(iy * sizeof(double *));

        for (j = 0; j < iy; j++) {
            A3D[i][j] = (double *)malloc(iz * sizeof(double));
            if(!(j+1 < iy))printf("alloc i=%d,j=%d\n",i,j); //debug
        }
    }
    return A3D;
}

void FreeArray3D(double *** A3D, int ix, int iy){
    ix++; iy++;
    int i, j;
    for (i = 0; i < ix; i++) {
        for (j = 0; j < iy; j++) {
            if(!(j+1 < iy))
                printf("free i=%d,j=%d\n",i,j); //wenn das der letzte durchlauf ist printe
            free(A3D[i][j]);
        }
        free(A3D[i]);
    }
    free(A3D);
}

int main(){
    int a=99, b=99;
    double *** A_xyz;
    A_xyz = AllocArray3D(a, b, 1309);
    FreeArray3D(A_xyz, a, b);
    return 0;
}

when i change one function to(note the change in the condition of the for loops):

double ***AllocArray3D(short ix, int iy, int iz){
    double ***A3D;//x++; iy++;
    A3D = (double ***)malloc(ix * sizeof(double **));
    int i, j;
    for (i = 0; i < (ix+1); i++) {
        A3D[i] = (double **)malloc(iy * sizeof(double *));

        for (j = 0; j < (iy+1); j++) {
            A3D[i][j] = (double *)malloc(iz * sizeof(double));
            if(!(j+1 < iy))printf("alloc i=%d,j=%d\n",i,j); //debug
        }
    }
    return A3D;
}

or (note the change in the condition of the for loops): EDIT: I forgot to comment out ix++; iy++;:

double ***AllocArray3D(short ix, int iy, int iz){
    double ***A3D; ix++; iy++;
    A3D = (double ***)malloc(ix * sizeof(double **));
    int i, j;
    for (i = 0; i <= ix; i++) {
        A3D[i] = (double **)malloc(iy * sizeof(double *));

        for (j = 0; j <= iy; j++) {
            A3D[i][j] = (double *)malloc(iz * sizeof(double));
            if(!(j+1 < iy))printf("alloc i=%d,j=%d\n",i,j); //debug
        }
    }
    return A3D;
}

I already checked that i can use the array and that every allocation is not NULL but the free() function is failing.

What did i miss? i would be very thankful for an explanation. Thanks in advance

If you change the value of iy (by not incrementing it), you have to change how it is used everywhere within the function. — Scott Hunter
– Scott Hunter, Commented Oct 18, 2024 at 13:48
The increments and the inclusive loops make in unclear what the array dimensions are supposed to be. — Weather Vane
– Weather Vane, Commented Oct 18, 2024 at 13:53
The original code is strange, in that it allocates a structure larger by one than is asked for in each of the first two dimensions. I could make sense of that if the extra positions were used for some special purpose, such as storing sentinel values, but that does not appear to be the case. And that's related to the changes you are trying to make, so it may even be relevant. — John Bollinger
– John Bollinger, Commented Oct 18, 2024 at 14:24
It'd be simpler to just do double(*A_xyz)[a][b][1309] = malloc(sizeof *A_xyz); and then access the result with (*A_xyz)[ix][iy][iz] (where ix, iy and iz are the indices). — Ted Lyngmo
– Ted Lyngmo, Commented Oct 18, 2024 at 14:25
@merlin When you change the condition to i < (ix+1) or i <= ix you know that you will access the array out of bounds, right? If ix == 1 you can only access element 0 in the array, but with i < (ix + 1) you will access element 0 and 1. This has undefined behavior and anything could happen. — Ted Lyngmo
– Ted Lyngmo, Commented Oct 18, 2024 at 14:35

John Bode · Accepted Answer · 2024-10-18 17:38:44Z

Let's look at this segment of code:

A3D = (double ***)malloc(ix * sizeof(double **));
int i, j;
for (i = 0; i <= ix; i++) {
    A3D[i] = (double **)malloc(iy * sizeof(double *));

You've sized A3D to hold ix elements, but you're allocating ix + 1 items; you've gone past the end of the array.

Similarly, you've sized A3D[i] to hold iy items, but you wind up allocating iy + 1 items, again running past the end of the array.

Bad juju.

If free is failing, it might mean you've overwritten some metadata associated with one or more of your dynamically-allocated blocks by going past the end of each array.

Some malloc implementations will add some extra bytes to the allocated buffer to store size (and possibly other) metadata so that free knows how much memory to deallocate (this is just an illustration of the concept, not meant to represent any real-world malloc implementation):

         +----+ ----+
 0xffed: | 00 |     |
         +----+     +-- 4 bytes allocated
 0xffef: | 04 |     |
         +----+ ----+
 0xfff0: | ?? | <------ this is the address returned by malloc
         +----+
 0xfff1: | ?? |
         +----+
 0xfff2: | ?? |
         +----+
 0xfff3: | ?? |
         +----+

So when you pass that address to free, it knows to look at the two bytes before that address to know how much memory to deallocate. If those bytes have been overwritten (say by going past the end of a previously allocated block), then that might be the cause of the problems you're seeing.

Maybe.

Point is, you're spilling over your array bounds and that's bad.

Just as a stylistic note:

In C, you do not need to cast the result of malloc; as of C89 it returns void *, and in C a void * can be assigned to any other object pointer type and vice versa without a cast (this is not true in C++, but if you're writing C++ you're not using malloc anyway).

Furthermore, you can make your target the operand of sizeof:

A3D = malloc( ix * sizeof *A3D );  // or sizeof A3D[0]
...
A3D[i] = malloc( iy * sizeof *A3D[i] ); // or sizeof A3D[i][0]

Makes things a bit less eye-stabby, and makes it easy to change the type of A3D (say from double to long double) without having to hack every stinking malloc call. You do not need to expose type information here, and you honestly shouldn't.

jmullee · Accepted Answer · 2024-10-18 15:08:07Z

You have to be careful what you're counting. Your first change '..when i change one function..' can be made to work by also increasing the size of memory allocated:

// when i change one function:
A3D = (double ***)malloc(ix * sizeof(double **));
int i, j;
for (i = 0; i < (ix+1); i++) {
    A3D[i] = (double **)malloc(iy * sizeof(double *));

// works:
A3D = (double ***)malloc((ix+1) * sizeof(double **));
int i, j;
for (i = 0; i < (ix+1); i++) {
    A3D[i] = (double **)malloc((iy+1) * sizeof(double *));

The second one seems to work bu I note it also leaks memory. https://onlinegdb.com/tH6vV5bp1

BAD1_AllocArray3D:BAD1 allocated i=100,j=100
Ok.
FreeArray3D:freeing i=100,j=100
Ok.
BAD2_AllocArray3D:BAD2 allocated i=101,j=101
Ok.
FreeArray3D:freeing i=100,j=100
Ok.

Collectives™ on Stack Overflow

malloc & free in C with multidimensional arrays in C; Whats wrong with the code

2 Answers 2

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related