For loop error with 2D dynamically allocated array

Question

I'm having an issue with writing values to my 2D array that I've dynamically allocated. It seems that it is writing values to other spots in the array when it should not be.

As far as I can tell I've allocated the memory correctly and I don't believe my iterations are off.

When I try defining an array as double KAB[3][15]={0.0} I do not have this problem.

In this example, obviously, I'm using specific lengths of the array, but I would like them to work when defined by the user. Any trouble shooting suggestions would be appreciated.

#include <stdlib.h>
#include <stdio.h>
#include <math.h>
#include <time.h>
#include <string.h>



int main( ) 
{

int ms=0,i=0,j=0,n=0;
double value=0;
    double **KAB;
    KAB = (double **) malloc(3 * sizeof(double **));//makes array of pointers
    for(i = 0; i < 15; i++)
    {
        KAB[i] =(double *) malloc(3 *sizeof(double*));//each pointer points to a certain number of doubles
    }

        for(i=0;i< 3;i++)
        {
            for(j=0;j< 15;j++)
            {
                KAB[i][j]=0.0;//each value is set to 0.
            }
        }

for(ms=0; ms < 3; ms++)
{   
    for(i=0; i<15;i++)
    {       
        value=0;
        for(n=0; n<5 ;n++)
        {
                value+=ms*1.0+1;    
        }
        KAB[ms][i]=value;

        printf("Step:%d MS:%d\n",i,ms);
        printf("KAB[0][7]=%lf KAB[1][7]=%lf KAB[2][7]=%lf\n",KAB[0][7],KAB[1][7],KAB[2][7]);
    }
    }

return 0;
}//ends main

I've included relevant output with some annotations.

MS:0 Step:0
KAB[0][7]=0.000000, KAB[1][7]=0.000000, KAB[2][7]=0.000000
MS:0 Step:1

Everything starts at 0. And the first value gets put in the right spot.

MS:0 Step:7
KAB[0][7]=5.000000, KAB[1][7]=0.000000, KAB[2][7]=0.000000

But Before the end of the ms=0 loop something is written to the second row of the array

MS:0 Step:11
KAB[0][7]=5.000000, KAB[1][7]=5.000000, KAB[2][7]=0.000000

During the third step of the ms=1 loop the first row gets over written

MS:1 Step:3
KAB[0][7]=10.000000, KAB[1][7]=5.000000, KAB[2][7]=0.000000

At the appropriate step, the second row, column seven value gets the correct value entered

MS:1 Step:7
KAB[0][7]=10.000000, KAB[1][7]=10.000000, KAB[2][7]=0.000000

But before the rest of the row is finished the same value gets put into the next row in the same column.

MS:1 Step:11
KAB[0][7]=10.000000, KAB[1][7]=10.000000, KAB[2][7]=10.000000

the second row gets replaced with some values from the third row

MS:2 Step:3
KAB[0][7]=10.000000, KAB[1][7]=15.000000, KAB[2][7]=10.000000

The third row gets it's correct value. These values remain until the end, but clearly the first and second rows have some incorrect values.

MS:2 Step:7
KAB[0][7]=10.000000, KAB[1][7]=15.000000, KAB[2][7]=15.000000

David C. Rankin · Accepted Answer · 2018-09-17 17:59:03Z

You are failing to allocate sufficient storage for each row after allocating for your pointers. (you have a magic number problem)

Before addressing the allocation issue, let's look a general issue regarding using magic numbers in your code. For example, in:

    KAB = (double **) malloc(3 * sizeof(double **));//makes array of pointers
    for(i = 0; i < 15; i++)

3 & 15 are magic numbers (meaning if anything changes and you need to adjust allocation or loop limits, you are left picking through your code to find each allocation and adjust each loop limit, each declaration size, etc...

Don't use magic numbers, e.g.

#define ROW     3   /* if you need a constant, #define one (or more) */
#define COL    15
#define NVAL    5
...
    for (int ms = 0; ms < ROW; ms++) {              /* for each row */

        kab[ms] = calloc (COL, sizeof *kab[ms]);    /* allocate storage */

In your case, you have the wrong magic number in your row allocation, e.g.

for(i = 0; i < 15; i++)
{
    KAB[i] =(double *) malloc(3 *sizeof(double*));
}

You only allocate 3-pointers, but you then attempt to allocate and assign storage for 3 double* to 15-pointers. Beginning with KAB[3] you invoke Undefined Behavior (because you have already used your 3 allocated pointers), and the defined operation of your code is over.

Using constants instead of magic numbers helps avoid this problem. Further, you have one convenient place to make changes at the top of you source file if anything needs changing.

Allocating Pointers and Storage for Values

When you are allocating using a pointer-to-pointer-to-type as your base type, you must

allocate a pointer to hold the memory address for each row; and
allocate storage for each row to hold the values

(you also must validate each allocation, malloc, calloc & realloc can and do fail on memory exhaustion)

Next, since you are looping to zero your kab row values to zero, just use calloc to allocate, it both allocates and set memory to zero.

(don't use UPPPER case variable names (reserved for constants and macros), and don't use camelCase or MixedCase variable - (leave those for java or C++))

So your allocation for both pointers and rows could look something like:

...
int main (void) {

    double **kab = NULL;

    /* calloc allocates and initializes memory all zero */
    kab = calloc (ROW, sizeof *kab);    /* use dereference pointer to
                                         * determine typesize */

    if (!kab) {     /* validate every allocation by checking the return */
        perror ("calloc-kab");  /* handle error */
        exit (EXIT_FAILURE);
    }

    for (int ms = 0; ms < ROW; ms++) {              /* for each row */

        kab[ms] = calloc (COL, sizeof *kab[ms]);    /* allocate storage */
        if (!kab[ms]) {                             /* validate allocation */
            perror ("calloc-kab[ms]");              /* handle error */
            exit (EXIT_FAILURE);
        }
...

(There is no need to cast the return of malloc, it is unnecessary. See: Do I cast the result of malloc?)

You now have both pointers and storage for your row values allocated, set to zero (by virtue of using calloc) and you are free to address and fill your values using 2D index notation.

Putting all the pieces together, you could do something similar to the following:

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

#define ROW     3   /* if you need a constant, #define one (or more) */
#define COL    15
#define NVAL    5

int main (void) {

    double **kab = NULL;

    /* calloc allocates and initializes memory all zero */
    kab = calloc (ROW, sizeof *kab);    /* use dereference pointer to
                                         * determine typesize */

    if (!kab) {     /* validate every allocation by checking the return */
        perror ("calloc-kab");  /* handle error */
        exit (EXIT_FAILURE);
    }

    for (int ms = 0; ms < ROW; ms++) {              /* for each row */

        kab[ms] = calloc (COL, sizeof *kab[ms]);    /* allocate storage */
        if (!kab[ms]) {                             /* validate allocation */
            perror ("calloc-kab[ms]");              /* handle error */
            exit (EXIT_FAILURE);
        }

        for (int i = 0; i < COL; i++ ) {    /* for each column */
            double value = 0;

            for (int n = 0; n < NVAL; n++)  /* loop NVAL times */
                value += ms * 1.0 + 1;      /* build value */

            kab[ms][i] = value;             /* assign value to kab[ms][i] */
        }
    }

    for (int ms = 0; ms < ROW; ms++) {      /* for each row */
        for (int i = 0; i < COL; i++)       /* for each col */
            printf (" %4.1lf", kab[ms][i]); /* output value */
        putchar ('\n');     /* tidy up */
        free (kab[ms]);     /* free row */
    }
    free (kab);             /* free pointers */

    return 0;
}

Example Use/Output

How you build the value to store in each column is a bit uninteresting, but for purposes of the example it is fine.

$./bin/arraydyn2d
  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0
 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0
 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0

Memory Use/Error Check

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.

It is imperative that you use a memory error checking program to insure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ valgrind ./bin/arraydyn2d
==15774== Memcheck, a memory error detector
==15774== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==15774== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==15774== Command: ./bin/arraydyn2d
==15774==
  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0  5.0
 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0 10.0
 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0 15.0
==15774==
==15774== HEAP SUMMARY:
==15774==     in use at exit: 0 bytes in 0 blocks
==15774==   total heap usage: 4 allocs, 4 frees, 384 bytes allocated
==15774==
==15774== All heap blocks were freed -- no leaks are possible
==15774==
==15774== For counts of detected and suppressed errors, rerun with: -v
==15774== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

Look things over and let me know if you have further questions.

Thank you for your very thorough response. I'll do my best to take your comments into consideration in my future coding practices.
@JMC glad to help. There is a lot to learn in C (you really never get done learning it -- it's more of a journey than a ride). So slow down, take it bit by bit and enjoy the journey -- the learning will come, and it will make you a better programmer, regardless what language you end up writing code in.

Anupam · Accepted Answer · 2018-09-16 00:46:44Z

0

You are not doing malloc properly.

KAB = (double **) malloc(3 * sizeof(double **));

This statement is wrong and will allocate an array of double * but you need an array of double * because each element inside KAB should be a double * to point to an array of doubles and not a double **

Similar is the case with the following statement

KAB[i] =(double *) malloc(3 *sizeof(double*));

Here, you are allocating an array of double * instead, it should be an arrray of doubles.

The following code would work correctly.

#include <stdlib.h>
#include <stdio.h>
#include <math.h>
#include <time.h>
#include <string.h>



int main() 
{

    int ms=0,i=0,j=0,n=0;
    double value=0;
    double **KAB;
    KAB = (double **) malloc(3 * sizeof(double *));//makes array of type double pointers
    for(i = 0; i < 15; i++)
    {
      KAB[i] =(double *) malloc(3 *sizeof(double));//each element in KAB points to an array of doubles
    }

      for(i=0;i< 3;i++)
      {
        for(j=0;j< 15;j++)
        {
            KAB[i][j]=0.0;//each value is set to 0.
        }
      }

    for(ms=0; ms < 3; ms++)
    {   
        for(i=0; i<15;i++)
        {       
          value=0;
          for(n=0; n<5 ;n++)
          {
                  value+=ms*1.0+1;    
          }
          KAB[ms][i]=value;

          printf("Step:%d MS:%d\n",i,ms);
          printf("KAB[0][7]=%lf KAB[1][7]=%lf KAB[2][7]=%lf\n",KAB[0][7],KAB[1][7],KAB[2][7]);
        }
    }
    return 0;
}//ends main

answered Sep 16, 2018 at 0:46

Anupam

794 bronze badges

3 Comments

Nic Over a year ago

This is why I like to use double **KAB = (double **) malloc(3 * sizeof(*KAB));

Anupam Over a year ago

Yeah! Clever Trick.

JMC Over a year ago

Thank you for pointing this out. Although when trying this I received the same issues from before. As user3121023 pointed out I was inconsistent with my number of rows vs cols when declaring each KAB[i]. The for loop should've been until i <3 instead of 15. Interestingly enough, leaving my original incorrect malloc statement works when going over the correct number of iterations. Thanks again both of you.

Collectives™ on Stack Overflow

For loop error with 2D dynamically allocated array

2 Answers 2

2 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest