Error when freeing memory

Question

I am writing some code to compute the sum of fibonacci values up to n, as stored in an array. For only certain values of n, I get an error on calling free().

Edit: This code should now compile.

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

long fib(long *fibs, int n);

int main(int argc, char *argv[]) {

  long num, sum;
  long n;
  long *fibs;

  if(argc < 2) {
    printf("Usage %s n\n", argv[0]);
    exit(EXIT_SUCCESS);
  }

  n = strtol(argv[1], NULL, 10);
  printf("%ld\n", n);

  printf("--Allocating memory\n");
  fibs = (long *) malloc(sizeof(long) * n);
  printf("--Memory allocated\n");

  fibs[0] = 1;
  fibs[1] = 1;

  sum = 0;
  for(int i = 0; i <= n; i++) {
    num = fib(fibs, i);
    sum += num;

    printf("%ld\n", num);
  }

  printf("%ld\n", sum);

  printf("--Freeing memory\n");
  free(fibs);
  printf("--Memory freed\n");
}

long fib(long *fibs, int n) {
  if((n == 0) || (n == 1)) {
    return 1;
  }

  fibs[n] = fibs[n - 1] + fibs[n - 2];
  return fibs[n];
}

For instance, when I call the program ./fibsum with n=5, I get a core dump.

what is `n` in main? please post a minimal compilable code, there are so many problems in your code — mazhar islam, Jul 27 '15 at 16:23
Do not cast the return of malloc. And you should initialise fibs when declaring it. Also this doesn't declare i so I'm not sure how it compiles — Tom Tanner, Jul 27 '15 at 16:36
You should probably be using `malloc(n * sizeof(long))` rather than `malloc(n * sizeof(n))`. — Dmitri, Jul 27 '15 at 16:54

score 2 · Answer 1 · edited May 23 '17 at 12:06

2

The lines

  fibs[n] = 1;

and

 fibs[n] = fibs[n - 1] + fibs[n - 2];

modify memory beyond the legal range. The legal range is fibs[0] - fibs[n-1]. Due to that, the program displays undefined behavior. In your case, the undefined behavior manifests in the form of problem in the call to free.

You may want to allocate one more element than you are currently allocating. Instead of

fibs = (long *) malloc(n * sizeof(n));

use

fibs = malloc((n+1) * sizeof(n));

See an SO post on why you should not cast the return value of malloc.

edited May 23 '17 at 12:06

Community

1
1

answered Jul 27 '15 at 16:26

R Sahu

204,454
14
159
270

Why is that, he used if((n == 0) || (n == 1)) which means n-1 will be index 1 and n-2 will be index 0 or am i missing something – Jul 27 '15 at 16:28
@CamelToe, I did not follow your question. – R Sahu Jul 27 '15 at 16:33
_modify memory beyond the legal range_ how? @RSahu – mazhar islam Jul 27 '15 at 16:57
1

@rakeb.void, the `for` loop in `main` goes from `0` to `n`, not `0` to `n-1`. As a consequence, the posted code will modify `fibs[n]`. – R Sahu Jul 27 '15 at 17:03
1

Don't cast `malloc` result, it's unnecessary! http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc – Jakub Rakus Jul 27 '15 at 19:23

score 1 · Answer 2 · answered Jul 27 '15 at 16:37

There are number of problems -

i is undeclared in main.
n is not declared in main.
And most important one this loop-
```
 for(i = 0; i <= n; i++)
```

As you allocate memory for n items but loop goes from 0 to n .You forgot it should be till n-1 .Thus behaviour is undefined.

As case you described (n=5 so loop should be from 0 to 4).

score 1 · Accepted Answer · 2015-07-27T17:08:18.707

Like i told you in the comments, you are overflowing when using <= instead of < in the loop. Take a look at the following example, this is by no means trying to be a "cleaned up" version of your code, I just made it work without changing too much.

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

int number = 300;

long* fib(long* fibs, int n)
{
    if (n == 0 || n == 1)
    {
        fibs[n] = 1;
    }
    else
    {
        fibs[n] = (fibs[n-1] + fibs[n-2]);
    }
    return fibs;
}

int main(int argc, char *argv[])
{
    long* fibs = (long*)malloc(number * sizeof(long));
    long sum = 0;

    for (int i = 0; i < number; ++i) //changed <= to < like i said in the comments
    {
        sum += fib(fibs, i)[i];
        printf("%d\n", sum);
    }

    printf("\n\nSum of everything: %ld\n", sum);
    free(fibs); //no problem

    return 0;
}

mazhar islam · Answer 4 · 2015-07-27T17:00:27.403

There are few problems in your code, some are typing mistake I guess, some are logical problem:

You forgot to type n and i
Never cast malloc, so instead of fibs = (long *) malloc(n * sizeof(n)); try fibs = malloc(n * sizeof(long));
In the for loop, instead of for(i = 0; i <= n; i++) use for(i = 0; i < n; i++)
sum += fib(fibs, i); here, sum is long but fib() returns long *, so change function defalcation long *fib(long *fibs, int n) to long fib(long *fibs, int n)

I update your code then run and get output : 12.

The modified code:

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

long fib(long *fibs, int n) {
   if((n == 0) || (n == 1)) {
      fibs[n] = 1;
   } else {
       fibs[n] = fibs[n - 1] + fibs[n - 2];
   }
   return fibs[n];
}

int main(int argc, char *argv[]) {
   long *fibs;
   long sum = 0;
   int n = 6, i;
   fibs = malloc(n * sizeof(long));
   long tem;

   for(i = 0; i < n; i++) {
       tem = fib(fibs, i);
       sum += tem;
   }

   printf("%ld\n", sum);
   free(fibs);
}

Error when freeing memory

4 Answers4