How to properly free dynamically allocated memory for an array of pointers to int's

Question

I need to know if I have used free() correctly while attempting to free dynamically allocated memory for an array of pointers to int's.

My code is modified from a code snippet out of one of my books and is the beginning of a program which requests temperature readings for three cities.

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

int main()
{
  int ctr, num, num2 = 0, ctr2 = 0;
  int* temps[3];
  for(ctr = 0; ctr < 3; ctr++)
  {
     puts("how many readings for the city?");
     scanf(" %d", &num);
     temps[ctr] = (int*)malloc(num * sizeof(int));
     num2 += num;
     while(ctr2 < num2)
     {
         puts(" what is reading? ");
         scanf(" %d", &temps[ctr][ctr2]);
         printf("echo: %d ", temps[ctr][ctr2]);

         ctr2++;
     }
  }

  for(ctr = 0; ctr < 3; ctr++)
  {
      free(temps[ctr]);
  }

  getchar();
  getchar();

  return (0);
}

I know that a pointer which is assigned memory using malloc() may have values assigned and accessed through a combination of a loop and array indexes. Hence I have assigned values from user input using the indexes of a two dimensional array, and need to know if I used free correctly. I know this is extremely sloppy coding and I am merely seeking to understand free correctly to prevent any potential memory leaks.

Answer 1

Please let me know if the following code would be considered acceptable:

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








int main()
{
  int ctr, num, ctr2 = 0;
  int * temps[3];
  for(ctr = 0; ctr < (int)(sizeof(temps)/sizeof(*temps)); ctr++)
  {
     puts("how many readings for the city?");
 if (!scanf(" %d", &num) || num <= 0) { printf("wrong number\n"); exit(1); }
 temps[ctr] = (int *) malloc(num * sizeof(*temps[ctr]));

     while(ctr2 < num)
     {


     puts(" what is reading? ");
     scanf(" %d", &temps[ctr][ctr2]);
     printf("echo: %d ", temps[ctr][ctr2]);

     ctr2++;
     }
     ctr2 = 0;




  }

  for(ctr = 0; ctr < (int)(sizeof(temps)/sizeof(*temps)); ctr++)
  {
      free(temps[ctr]);
  }

  getchar();
  getchar();

  return (0);


 }

Answer 2

The proper way of freeing the dynamically allocated memory is to free it after you check if it has been allocated at the very first place or not. As your loop structure is same for allocating and freeing, there wont be any problem here as such if all the allocations are successful. Therefore I suggest you check at all the places if the allocation is successful after allocating and also check if the memory is allocated before freeing.

Following code will make sure all the cases:

scanf(" %d", &num);

/*
 * check here if the value of ctr in non-negative and in the appropriate range
 */

temps[ctr] = (int*)malloc(num * sizeof(int));
if (temps[ctr] == NULL) {
      printf ("Memory allocation failed\n");
      /* 
       * appropriate error handling
       */
}

Also, check when you are freeing the memory to be on the safer side.

for(ctr = 0; ctr < 3; ctr++)
{
      if(temps[ctr]) {
           free(temps[ctr]);
      }
}

Also there is a bug in your code where after the first iteration itself you will get memory out of bound error, as the variable ctr2 in never reinitialized.

num2 += num;
while(ctr2 < num2)
 {
     puts(" what is reading? ");
     scanf(" %d", &temps[ctr][ctr2]);
     printf("echo: %d ", temps[ctr][ctr2]);

     ctr2++;
 }

Here if the value of num was 20 in the first iteration, then in the second iteration you will end up taking the input starting from temps[1][20], and assuming the value of num in the second iteration to be 5, you have allocated just 5 * sizeof(int)), so obviously you are going out of bounds when you try to access temps[1][20].

Answer 3

it's okay since you respect the same number of loops for allocation and deallocation with the same statement:

for(ctr = 0; ctr < 3; ctr++)

Just make sure that temps can hold at least 3 elements, which is the case, and that num is not zero or undefined (test return value of scanf & value of num). You can use a sizeof formula in your case to avoid hardcoding the value, only because you have an array of pointers, not pointers on pointers.

also avoid casting return value of malloc. And use the size of the element, instead of hardcoding as int (so if type of the pointer changes, your sizes are still right). Improvement suggestion for allocation:

for(ctr = 0; ctr < (int)(sizeof(temps)/sizeof(*temps)); ctr++)
  {
     puts("how many readings for the city?");
     if (!scanf(" %d", &num) || num <= 0) { printf("wrong number\n"); exit(1); } // or better error handling
     temps[ctr] = malloc(num * sizeof(*temps[ctr]));

You may still get a segmentation fault when calling free if you corrupt the memory when filling your arrays (a comment suggests it does, since num2 grows and grows). If you get such errors, run your code with valgrind, or just perform allocations/deallocations (and not the rest) to find which part of the code causes the problem.