Freeing two dimensional array of stack

This is the edited code:

I have a two dimensional stack, like

#define push(s,ele) s.list[++(s.last)]=ele

typedef struct vp {
    short int v1,v2;
}VPTYPE;  

typedef struct VPLIST{
    int last;
    VPPTR *list;
}VPLISTTYPE,*VPLISTPTR ;    

VPLISTPTR v1v2;  
v1v2=(VPLISTPTR)malloc(sizeof(VPLISTTYPE)*nof);  
a=0;
while(a<100)
{  //allocation part
   for(i=0;i< nof;i++)  
   {  
      v1v2[i].list=(VPPTR *)malloc(20*(sizeof(VPPTR)));  

     for(i2=0;i2< 10;i2++) //plea开发者_开发百科se note that I am not filling the array completely, in the actual code the value 10 is dependent on certain factors, which I am omitting for the sake of simplicty 
     {  
         v=(VPTYPE *)malloc(sizeof(VPTYPE));  
         push(v1v2[i],v); 
         v1v2[i]->v1=1;v1v2[i]->v2=2;
     }
   }
   // some algorithm goes on here which accesses these values in the stack

   // free memory part
  for(i=0;i< nof;i++)  
   {  
      for(i2=0;i2<= (v1v2[i2].last);i2++)
     {  
         free(v1v2[i2].list[i]); 
     }
   }

   a++;
}

When I free the memory this way there is memory leakage. Please let me know where I am going wrong.

Thank you very much.

• You are not initializing the allocated memory in the code you show. In general, you get non-zero garbage allocated by malloc(). If you need zeroed memory, use calloc().

• There's also the problem identified by JCooper's answer.

• There's also the problem identified by Muggen's comment.

• You are freeing the items on the stack, but not the stacks as a whole. That should be done inside the 'for (i2 = 0; ...)' loop but after the 'for (k2 = 0; ...)' loop.

Collectively, these add up to a minor catastrophe.

After the code edit...

• The type VPPTR is not defined, but is presumably meant to be 'typedef VPTYPE *VPPTR;.
• In the struct VPLIST, you have a pointer to a VPPTR - one more reason to distrust such pointer typedefs. You almost certainly intended to have a simple VPPTR there. However, the other code does assume you need an array of pointers to pointers, so it is self-consistent (up to a point).
• This problem propagates into the memory allocation code.

• In your free memory loop, in the call to free(), you have reversed the roles of i and i2:

  free(v1v2[i2].list[i]);  // Yours
  free(v1v2[i].list[i2]);  // Mine
  

• Your assignments in the allocation loop (v1v2[i]->v1=1;v1v2[i]->v2=2;) are bogus.

The following code compiles clean and runs clean:

$ cc -Wall -Wextra -g -O3 -std=c99 x.c -o x
$ valgrind ./x
==16593== Memcheck, a memory error detector.
==16593== Copyright (C) 2002-2006, and GNU GPL'd, by Julian Seward et al.
==16593== Using LibVEX rev 1658, a library for dynamic binary translation.
==16593== Copyright (C) 2004-2006, and GNU GPL'd, by OpenWorks LLP.
==16593== Using valgrind-3.2.1, a dynamic binary instrumentation framework.
==16593== Copyright (C) 2000-2006, and GNU GPL'd, by Julian Seward et al.
==16593== For more details, rerun with: -v
==16593== 
==16593== 
==16593== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 5 from 1)
==16593== malloc/free: in use at exit: 0 bytes in 0 blocks.
==16593== malloc/free: 2,201 allocs, 2,201 frees, 40,032 bytes allocated.
==16593== For counts of detected errors, rerun with: -v
==16593== All heap blocks were freed -- no leaks are possible.
$

Working Code

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

#define push(s, ele) ((s).list[((s).last)++] = (ele))

typedef struct vp
{
    short v1;
    short v2;
} VPTYPE;

typedef struct VPLIST
{
    int     last;
    VPTYPE **list;
} VPLISTTYPE;

enum { nof = 2 };

int main(void)
{
    VPLISTTYPE *v1v2 = (VPLISTTYPE *)malloc(sizeof(*v1v2) * nof);

    for (int i = 0; i < nof; i++)
        v1v2[i].last = 0;

    for (int a = 0; a < 100; a++)
    {
        //allocation part
        for (int i = 0; i < nof; i++)
        {
            v1v2[i].list = (VPTYPE **)malloc(20 * sizeof(*v1v2[i].list));

            for (int i2 = 0; i2 < 10; i2++)
            {
                VPTYPE *v = (VPTYPE *)malloc(sizeof(*v));
                v->v1 = 1;
                v->v2 = 2;
                push(v1v2[i], v);
            }
        }

        // free memory part
        for (int i = 0; i < nof; i++)
        {
            for (int i2 = 0; i2 < (v1v2[i].last); i2++)
            {
                free(v1v2[i].list[i2]);
            }
            free(v1v2[i].list);
            v1v2[i].list = 0;
            v1v2[i].last = 0;
        }
    }
    free(v1v2);
    return 0;
}

Simpler Working Code

This code uses one less level of indirection - and one less level of memory allocation, therefore - and compiles and runs equally cleanly.

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

#define push(s, ele) ((s).list[((s).last)++] = (ele))

typedef struct vp
{
    short v1;
    short v2;
} VPTYPE;

typedef struct VPLIST
{
    int     last;
    VPTYPE *list;
} VPLISTTYPE;

enum { nof = 2 };

int main(void)
{
    VPLISTTYPE *v1v2 = (VPLISTTYPE *)malloc(sizeof(*v1v2) * nof);

    for (int i = 0; i < nof; i++)
        v1v2[i].last = 0;

    for (int a = 0; a < 100; a++)
    {
        //allocation part
        for (int i = 0; i < nof; i++)
        {
            v1v2[i].list = (VPTYPE *)malloc(20 * sizeof(*v1v2[i].list));

            for (int i2 = 0; i2 < 10; i2++)
            {
                VPTYPE v;
                v.v1 = 1;
                v.v2 = 2;
                push(v1v2[i], v);
            }
        }

        // free memory part
        for (int i = 0; i < nof; i++)
        {
            free(v1v2[i].list);
            v1v2[i].list = 0;
            v1v2[i].last = 0;
        }
    }
    free(v1v2);
    return 0;
}

Unless I misunderstand the code, it looks like when you push, because you use the pre-increment, the .last field actually holds the index of the last thing, not a count of how many have been pushed. However, when you loop through to free them, you're looping while less than last, not less than or equal.