Can't deallocate pointer to structure previously allocated

Question

I'm creating easy particle system. I've got two typedef structures. First represents single particle with some fields. Second represents system of particles. My problem is that I can't deallocate memory allocated for each Particle. Really don't know what's wrong. Here are structures:

typedef struct {
   float m;  
   float *x;  
   float *v;    
   float *f;     
   float R;
} *Particle;

typedef struct {
   Particle *p;
   int n; 
   float t; 
} *ParticleSystem;

and here code for allocating

ParticleSystem sys = (ParticleSystem) malloc(sizeof(ParticleSystem));  
sys->p =  (Particle *) malloc(sizeof(Particle)*noOfParticles);

for(int i=0;i<noOfParticles;i++){
    sys->p[i] = (Particle)malloc(sizeof(Particle));

    sys->p[i]->f = (float*)malloc(sizeof(float)*2);
    sys->p[i]->f[0] = 0.0f;
    sys->p[i]->f[1] = 0.0f;

    ...

    sys->p[i]->R = radius;
    sys->p[i]->m = mass;

}

sys->n=noOfParticles;
sys->t = 0.0f;

and freeing

int n = sys->n;
for(int i=0;i<n;i++){
    free(sys->p[i]->f);

    ...

    free(sys->p[i]);//here it breaks
}
free(sys->p);
free(sys);

At line "free(sys->p[i]) it breaks. I don't know why, because first I do this
sys->p[i] = (Particle)malloc(sizeof(Particle)) to allocate. Visual Studio says "HEAP[template.exe]: Invalid address specified to RtlValidateHeap( 01E70000, 01E749B0 ) Windows has triggered a breakpoint in template.exe."

Answer 1

It's been a while since I've done C/C++ programming, but don't those typedefs declare Particle to be of type pointer-to-that-struct and ParticleSystem to be of type pointer-to-that-other-struct? If so, when you allocate the memory you're only allocating enough memory to hold a pointer, not the actual struct.

For example:

#include <iostream>

using namespace std;

typedef struct {
  float m;
  float *x;
  float *v;
  float *f;
  float R;
} *Particle;

typedef struct {
  Particle *p;
  int n;
  float t;
} *ParticleSystem;

int main()
{
  cout << "sizeof(Particle) = " << sizeof(Particle) << endl;
  cout << "sizeof(ParticleSystem) = " << sizeof(ParticleSystem) << endl;
  return 0;
}

and when I run that program I get:

sizeof(Particle) = 4
sizeof(ParticleSystem) = 4

Answer 2

I would not use an asterisk when making such a typedef. This easily leads to a lot of confusion when malloc'ing memory and using pointers. Just use

typedef struct {
   float m;  
   float *x;  
   float *v;    
   float *f;     
   float R;
} Particle;

This will make it much more clear, I don't even know which behaviour your typedef causes (your probably only alloc space for a pointer)

Just take a line at those 2 lines:

sys->p =  (Particle *) malloc(sizeof(Particle)*noOfParticles);
sys->p[i] = (Particle)malloc(sizeof(Particle));

I assume you want to get memory for the pointers in the first place and than memory for the struct itself in the second. But how should this work? You use sizeof(Particle) in both mallocs.

Answer 3

If this is C, you really shouldn't be type-casting the return value of malloc(). If this is C++, you really shouldn't be using malloc().

Also, I would suggest not typedef:ing away your pointers, it makes it much harder to follow the code and spot bugs.

Further, there's no point in using malloc() if you know that an array will always be of a constant small size, such as two (Particle.f is used this way). Just declare an array directly, and save on the malloc()s and associated confusion.

Answer 4

Try changing:

typedef struct {
float m;  
float *x;  
float *v;    
float *f;     
float R;
} *Particle;

to

typedef struct {
float m;  
float *x;  
float *v;    
float *f;     
float R;
} Particle;

You need to have a Particle in order to have a *Particle and in order to have sizeof(Particle) work... The same goes for ParticleSystem

Answer 5

I would start by fixing the struct declaration to:

} Particle;

and

} ParticleSystem;

instead of pointers.

And then update the rest of code to reflect this changes, beginning with:

ParticleSystem* sys = (ParticleSystem*) malloc(sizeof(ParticleSystem));  
sys->p =  (Particle*) malloc(sizeof(Particle) * noOfParticles);

Answer 6

First, you have Particle *p, that you can allocate as a single Particle or an array of Particles.

You are allocating *p as an array in the second line, so you must NOT allocate them again inside the for loop, UNLESS you use Particle **p and then allocate *(p) as an array os POINTERS to Particle, then you alloc each particle inside the for loop. I do not recommend that.

As mentioned above, remove the pointer declaration from the structs.

And, following that you don't have to re-allocate every particle (since p is an ordinary pointer), you should free it with free(sys->p); only, because you're allocating each single particle as a direct place in the memory, not as pointers, as I said.

Hope it helps :)