Valgrind reports memory leak for realloc call. But unable to understand why

Question

I have a struct as follows:

typedef struct somefruits{
 struct pine abc;
 int xyz;
}pm;

struct pine 
{
    unsigned char *ch;
    unsigned char *data;
    unsigned int a;
}

int function(unsigned int nfruits, pm **outfruits)
 {
  pm *fruits = NULL;
  status = 0;
  void *tmp = NULL;
  tmp = realloc(fruits, (nfruits + 1) * sizeof *fruits);
  if (tmp != NULL)
  fruits = tmp;

  //do some manipulations on fruits. malloc and assign values for ch and data;

   for (i =0 ;i<nfruits;i++)
   {
       // do some stuff
       fruits[i]->abc.data = malloc(20);
       //write some stuff into data

       fruits[i]->abc.ch = malloc(100);
        //write some stuff into ch 
   }
  //do manipulations which can result in change of status variable 

  *outfruits =  fruits;

 return status;
}

 int main ()
 {
    pm *Fmain;
    function(10, &Fmain);

     // do some stuff with Fmain
    dispose(Fmain, 10);
 }

 dispose(pm *object, int n)
 {
   for(i=0;i<n;i++)
   {
       free((object+i)->abc.ch);
       free ((object+i)->abc.data);
   }
     free (object);
 }

When I run valgrind on the following code I get

    HEAP SUMMARY:
  ==4699== 421 (352 direct, 69 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 4
  ==4699==    by 0x4A079DA: realloc 
  ==4699==    by 0x40A065: function (file1.c:623)

Line 623 points to tmp = realloc(fruits, (nfruits + 1) * sizeof *fruits) line

Since I am freeing the memory in dispose I am unable to understand what it is that I am doing wrong. Any help will be much appreciated.

@KeithSmith Added clarification in the question.Line 623 points to `tmp = realloc(fruits, (nfruits + 1) * sizeof *fruits)` — user1357576, Dec 02 '13 at 03:08
Does this code compile? What compiler are you using? It doesn't work for me with gcc (eg, the parameter to `function` has type `pm**` and not `struct pm**`, `tmp` doesn't seem to be defined). — alecbz, Dec 02 '13 at 03:16
@alecbenzer I just added the relevant parts of the code. Let me add more info to not have any confusion. — user1357576, Dec 02 '13 at 03:19
You allocate N+1 items in `'function'. Where is `xyz` and `a` freed? Why is `object` freed in `dispose`? `object` is a pointer on the stack. — KeithSmith, Dec 02 '13 at 03:19
Do you really have a function called `function`? Please don't... — Floris, Dec 02 '13 at 03:20
@Floris no I don't.. But for simplicity purpose I used it here :) — user1357576, Dec 02 '13 at 03:25
@KeithSmith object is a pointer on stack. That makes sense. I guess I should be calling `dispose (&Fmain)`? and in free, should it be like `free (*object)` ? — user1357576, Dec 02 '13 at 03:29
Ah...Now I see a problem...Some malloc/free implementations require that the pointer passed to `free` be the same as the pointer returned from `malloc/realloc`. You are freeing the individual items. Just free the pointer returned from `function`. You allocate all the memory in one call. Free it in one call. — KeithSmith, Dec 02 '13 at 03:34
That's a very unconventional way to define the return type of the function. Normally, you'd define the structure type (with a semicolon at the end of the definition), and then use `struct pine` as the return type — and you'd need to return something. Alternatively, you still need the semicolon after the structure definition and the function should be defined to return `void`. — Jonathan Leffler, Dec 02 '13 at 03:40
@KeithSmith freeing in one call meaning, not doing `free((object+i)->ch);` and `free ((object+i)->data);` results in valgrind complaining about the leak of these two memory locations. So it has to be done! — user1357576, Dec 02 '13 at 03:44
Yes. Always match equal number of malloc/realloc with free. But you allocate N+1 and free N. One malloc/realloc, one free. Replace `dispose()` with `free(Fmain)` and see what Valgrind says. — KeithSmith, Dec 02 '13 at 03:49
@JonathanLeffler I saw that all the elements were allocated at once. I was suggesting to just free them at once instead of the loop of freeing the individual elements. I guess I was not clear in my comments. This is C, not C++. — KeithSmith, Dec 02 '13 at 04:00
@JonathanLeffler The ground is changing under our feet as we speak :) — KeithSmith, Dec 02 '13 at 04:03
[Not real code](http://codepad.org/cJZ7fPts). Respect others, don't waste their time. Post something that compiles. This is not reconstructmycodefromvaguehints.com. — n. m. could be an AI, Dec 02 '13 at 04:03
@n.m. I am not sure if others can actually make sense of a 1000 line code when you specifically know the issue is in certain x lines of code. :) — user1357576, Dec 02 '13 at 04:08
Reduce to 100 lines of valid code that reproduces the problem. This is your part of the deal. — n. m. could be an AI, Dec 02 '13 at 04:14
[This](http://codepad.org/SBOzSsGs) doesn't seem to have any problems with memory leaks. — n. m. could be an AI, Dec 02 '13 at 04:45

Jonathan Leffler · Answer 1 · 2013-12-02T04:01:49.700

Before the question was rewritten!

It really isn't all that hard. This code compiles cleanly; it also runs cleanly under valgrind. Note that the definitions of struct pine and struct somefruit had to be reversed compared to the question, amongst other minutiae.

#include <stdlib.h>

struct pine
{
    unsigned char ch;
    unsigned char data;
    unsigned int a;
};

typedef struct somefruits
{
    struct pine abc;
    int xyz;
} pm;

void function(unsigned nfruits, pm **outfruits);
void dispose(pm * object);

void function(unsigned nfruits, pm **outfruits)
{
    pm *fruits = malloc((nfruits + 1) * sizeof *fruits);
    if (fruits != NULL)
    {
        for (unsigned i = 0; i < nfruits + 1; i++)
        {
            fruits[i].xyz = i;
            fruits[i].abc.ch = i + 'A';
            fruits[i].abc.data = i + 'a';
            fruits[i].abc.a = i + 237;
        }
    }
    *outfruits =  fruits;
}

void dispose(pm * object)
{
    free(object);
}

int main(void)
{
    pm *Fmain;
    function(10, &Fmain);

    dispose(Fmain);
    return 0;
}

After the question was rewritten!

#include <stdlib.h>

struct pine
{
    unsigned char *ch;
    unsigned char *data;
    unsigned int a;
};

typedef struct somefruits
{
    struct pine abc;
    int xyz;
} pm;

void function(unsigned nfruits, pm **outfruits);
void dispose(unsigned nfruits, pm *object);

void function(unsigned nfruits, pm **outfruits)
{
    pm *fruits = malloc((nfruits + 1) * sizeof *fruits);
    if (fruits != NULL)
    {
        for (unsigned i = 0; i < nfruits + 1; i++)
        {
            fruits[i].xyz = i;
            fruits[i].abc.ch = malloc(20);
            fruits[i].abc.data = malloc(20);
            fruits[i].abc.a = i + 237;
            if (fruits[i].abc.ch != 0)
                fruits[i].abc.ch[0] = '\0';
            if (fruits[i].abc.data != 0)
                fruits[i].abc.data[0] = '\0';
        }
    }
    *outfruits =  fruits;
}

void dispose(unsigned nfruits, pm *object)
{
    for (unsigned i = 0; i < nfruits + 1; i++)
    {
        free(object[i].abc.ch);
        free(object[i].abc.data);
    }
    free(object);
}

int main(void)
{
    pm *Fmain;
    function(10, &Fmain);

    dispose(10, Fmain);
}

The nonsense with nfruits + 1 is downright dangerous. The code is consistent, but the +1 is not good design. This code too has a clean bill of health from valgrind.

I had missed some vital details. Added them to the code for better understanding. Very sorry for the misleading post — user1357576, Dec 02 '13 at 03:56

Valgrind reports memory leak for realloc call. But unable to understand why

1 Answers1

Before the question was rewritten!

After the question was rewritten!