1

I have a function which is called a lot of times during the execution. Inside this function I allocate an array:

double **MUDG_table;

//dynamic allocate array of MUDG_table (1st Dimension)
MUDG_table = calloc(arr_row,sizeof(double *));

//check if the memory has been allocated correctly
if (MUDG_table==NULL) 
{
    printf("Error allocating memory!\n"); //print an error message
    return 1; //return with failure
}

for (cv02=0;cv02<arr_row;cv02++)
{
    MUDG_table[cv02] = calloc(arr_column, sizeof(double));

    //check if the memory has been allocated correctly
    if (MUDG_table[cv02]==NULL) 
    {
        printf("Error allocating memory!\n"); //print an error message
        return 1; //return with failure
    }
}

When I finish with the the calculations and before returning the value I want I try to free the memory:

//free memory
for (cv02=0;cv02<arr_row;cv02++)
{
    free(MUDG_table[cv02]);
}
free(MUDG_table);

and it crashes.

If I remove it, it works a few times (as I said the function is called several times in a loop with different arguments each time) and then it crashes. Any ideas?

Yu Hao
  • 119,891
  • 44
  • 235
  • 294
kat
  • 97
  • 7
  • 1
    Please post the code between filling the data and freeing it. Why are you using `calloc()` you leave a lot of values with `0`? – Iharob Al Asimi Apr 28 '15 at 13:36
  • @iharob Do you suggest to try with malloc? Up to this point all my arrays are with calloc and I don't have a problem. – kat Apr 28 '15 at 13:39
  • 7
    Does it crash also if you don't call the calculation part ? If no, then you probably have buffer over/underflows somewhere in the calculation part. BTW `calloc` is OK. No need to change `calloc` to `malloc`. – Jabberwocky Apr 28 '15 at 13:39
  • As I told you in your [previous question](http://stackoverflow.com/q/29897609/1151654) you should not write `sizeof(double*)` but rather `sizeof(*MUDG_table)`. Have you tried using a debugger such as gdb to see where your program crashes and why? (E.G. you are calling `free` on a garbage value rather than on a pointer you really alloc'd) – Eregrith Apr 28 '15 at 13:41
  • @Eregrith Is type of `*MUDG_table` not the same as type of `double*`? – Maxim Egorushkin Apr 28 '15 at 13:43
  • @MaximEgorushkin Not if you ever change the declaration of `MUDG_table` and it's less error prone as you never have to remember your variable's type and think it through. Always write as `X = malloc(sizeof(*X) *...);`, with X declared as anything from `*x` to `****x[y][z][alpha]` – Eregrith Apr 28 '15 at 13:46
  • 3
    @kat, `calloc()` is good if you are **not going to** explicitly initialize the values, otherwise `malloc()` is faster. Also, `calloc()` hides some errors when debugging with memory debuggers. – Iharob Al Asimi Apr 28 '15 at 13:46
  • @MichaelWalz I took Ok, If I do the allocation, without performing any calculations, and in the end freeing the memory it doesn't seem to have an overflow. So I guess i missing sth in the values I assign to the array (storing values from a file). Despite this, the allocation and the freeing seems ok, correct? – kat Apr 28 '15 at 13:48
  • @kat yes, they are not the cause. – Iharob Al Asimi Apr 28 '15 at 13:49
  • @Eregrith In other words the type is the same. If you need it less error prone use C++. – Maxim Egorushkin Apr 28 '15 at 13:52
  • @MaximEgorushkin C++ has nothing to do with being "less error prone". In other words, needing to go through the code, find all the `sizeof`s and change them to the correct type, having to remember what type is the variable, is a pain. Just write `sizeof(*var)` and be done with it... [Look here for a more detailed list of reasons](http://stackoverflow.com/questions/17258647/why-is-it-safer-to-use-sizeofpointer-in-malloc) – Eregrith Apr 28 '15 at 13:55
  • 3
    Michael Walz's suggestion is the first thing to look into: Check whether your calculation code accesses the allocated array with an illegal index, most likely one or two beyond the upper bound. A memory checking tool will find such illegal accesses for you. Valgrind is such a tool for Linux. – M Oehm Apr 28 '15 at 14:10
  • @MOehm Yeh, I know. I am on it and trying to figure out... When I have something I will post it. Thank you all for the suggestions and help. – kat Apr 28 '15 at 14:17
  • You might want to use a real [2D array](http://stackoverflow.com/questions/12462615/how-do-i-correctly-set-up-access-and-free-a-multidimensional-array-in-c) instead of this pointer-to-pointer lookup table fragmented all over the heap. At the same time as you get a real array instead of the garage hacked lookup-table, you also get rid of bugs like this. – Lundin Apr 28 '15 at 14:50
  • @Lundin I think you are right. It will be less complicated and easier to control – kat Apr 28 '15 at 15:00

1 Answers1

1

You haven't provided enough information for us to determine what is causing the error, as the code you presented looks ok as far as it goes. Chances are good that either one or more of your pointers is getting corrupted, or that arr_row is being increased between allocation and deallocation.

You may be going to a lot more trouble than you need to do, however. IF

  • your program can rely on features that were new in C99, and
  • the dimensions of your array do not need to change during any one run of the function containing it (i.e. there is no reallocation)

then it would be much simpler to use a variable-length array:

double MUDG_table[arr_row][arr_column];

That won't necessarily solve your root problem, but it will surely simplify your code, and may make it easier to recognize the root problem.

John Bollinger
  • 160,171
  • 8
  • 81
  • 157
  • @ John Bollinger The problem is here: [link](http://stackoverflow.com/questions/29897609/c-programming-read-double-numbers-from-text-file-line-by-line-to-a-2d-array/29913737#29913737) In the while tok!=NULL. I Increase the columns more times than I should. – kat Apr 28 '15 at 15:03
  • As far as I can tell, the referenced function, as written, satisfies the criteria I gave for using a VLA. If there are input lines containing more numbers than you expect, then you have at least three alternatives: ignore the excess values, fail, or reallocate to make more space. The last is not possible with a VLA. You could, however, allocate an array big enough to hold any possible result. In the case of the code you referenced, there is no way you could parse more than 512 whitespace-delimited numbers from a 1024-byte buffer. – John Bollinger Apr 28 '15 at 15:37