2019-10-16 11:56 EDT


View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0002421FSSCPscriptingpublic2011-06-01 15:41
Reporterm_m 
Assigned ToThe_E 
PrioritynormalSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
Product Version3.6.13 
Target VersionFixed in Version3.6.13 
Summary0002421: Pointer to a particle returned by particle_create() becomes invalid
DescriptionWhen a particle is deleted in particle_move_all the memory is filled with random data. I attached a patch that introduces a signature for a particle that gets reset when the particle is removed from the vector making it possible to identify deleted and invalid particles. The new code also uses direct heap allocation to create the particle objects so that's something a coder should look at.
Additional InformationI compiled and tested the code using Visual Studio 2010.
TagsNo tags attached.
Attached Files
  • patch file icon particleFix_r7099.patch (14,133 bytes) 2011-03-31 11:30 -
    Index: code/parse/lua.cpp
    ===================================================================
    --- code/parse/lua.cpp	(revision 7100)
    +++ code/parse/lua.cpp	(working copy)
    @@ -8912,6 +8912,7 @@
     {
     protected:
     	particle *part;
    +	uint sig;
     public:
     	particle_h()
     	{
    @@ -8921,6 +8922,8 @@
     	particle_h(particle *particle)
     	{
     		this->part = particle;
    +		if (particle != NULL)
    +			this->sig = particle->signature;
     	}
     
     	particle* Get()
    @@ -8930,8 +8933,15 @@
     
     	bool isValid()
     	{
    -		return this != NULL && part != NULL;
    +		if (this != NULL && part != NULL && part->signature != 0 && part->signature == this->sig)
    +			return true;
    +		else
    +			return false;
     	}
    +
    +	~particle_h()
    +	{
    +	}
     };
     
     //**********HANDLE: Particle
    @@ -8944,6 +8954,9 @@
     	if (!ade_get_args(L, "o|o", l_Particle.GetPtr(&ph), l_Vector.Get(&newVec)))
     		return ade_set_error(L, "o", l_Vector.Set(vmd_zero_vector));
     
    +	if (ph == NULL)
    +		return ade_set_error(L, "o", l_Vector.Set(vmd_zero_vector));
    +
     	if (!ph->isValid())
     		return ade_set_error(L, "o", l_Vector.Set(vmd_zero_vector));
     
    @@ -8961,6 +8974,9 @@
     	vec3d newVec = vmd_zero_vector;
     	if (!ade_get_args(L, "o|o", l_Particle.GetPtr(&ph), l_Vector.Get(&newVec)))
     		return ade_set_error(L, "o", l_Vector.Set(vmd_zero_vector));
    +	
    +	if (ph == NULL)
    +		return ade_set_error(L, "o", l_Vector.Set(vmd_zero_vector));
     
     	if (!ph->isValid())
     		return ade_set_error(L, "o", l_Vector.Set(vmd_zero_vector));
    @@ -8979,6 +8995,9 @@
     	float newAge = -1.0f;
     	if (!ade_get_args(L, "o|f", l_Particle.GetPtr(&ph), &newAge))
     		return ade_set_error(L, "f", -1.0f);
    +	
    +	if (ph == NULL)
    +		return ade_set_error(L, "f", -1.0f);
     
     	if (!ph->isValid())
     		return ade_set_error(L, "f", -1.0f);
    @@ -8998,6 +9017,9 @@
     	float newLife = -1.0f;
     	if (!ade_get_args(L, "o|f", l_Particle.GetPtr(&ph), &newLife))
     		return ade_set_error(L, "f", -1.0f);
    +	
    +	if (ph == NULL)
    +		return ade_set_error(L, "f", -1.0f);
     
     	if (!ph->isValid())
     		return ade_set_error(L, "f", -1.0f);
    @@ -9017,6 +9039,9 @@
     	float newRadius = -1.0f;
     	if (!ade_get_args(L, "o|f", l_Particle.GetPtr(&ph), &newRadius))
     		return ade_set_error(L, "f", -1.0f);
    +	
    +	if (ph == NULL)
    +		return ade_set_error(L, "f", -1.0f);
     
     	if (!ph->isValid())
     		return ade_set_error(L, "f", -1.0f);
    @@ -9036,6 +9061,9 @@
     	float newTracer = -1.0f;
     	if (!ade_get_args(L, "o|f", l_Particle.GetPtr(&ph), &newTracer))
     		return ade_set_error(L, "f", -1.0f);
    +	
    +	if (ph == NULL)
    +		return ade_set_error(L, "f", -1.0f);
     
     	if (!ph->isValid())
     		return ade_set_error(L, "f", -1.0f);
    @@ -9055,6 +9083,9 @@
     	object_h *newObj;
     	if (!ade_get_args(L, "o|o", l_Particle.GetPtr(&ph), l_Object.GetPtr(&newObj)))
     		return ade_set_error(L, "o", l_Object.Set(object_h()));
    +	
    +	if (ph == NULL)
    +		return ade_set_error(L, "o", l_Object.Set(object_h()));
     
     	if (!ph->isValid())
     		return ade_set_error(L, "o", l_Object.Set(object_h()));
    @@ -9073,6 +9104,9 @@
     	particle_h *ph = NULL;
     	if (!ade_get_args(L, "o", l_Particle.GetPtr(&ph)))
     		return ADE_RETURN_FALSE;
    +	
    +	if (ph == NULL)
    +		return ADE_RETURN_FALSE;
     
     	return ade_set_args(L, "b", ph->isValid());
     }
    Index: code/particle/particle.cpp
    ===================================================================
    --- code/particle/particle.cpp	(revision 7100)
    +++ code/particle/particle.cpp	(working copy)
    @@ -23,7 +23,7 @@
     #endif
     
     int Num_particles = 0;
    -static SCP_vector<particle> Particles;
    +static SCP_vector<particle*> Particles;
     
     int Anim_bitmap_id_fire = -1;
     int Anim_num_frames_fire = -1;
    @@ -38,7 +38,9 @@
     
     static const int Min_particle_bump = 200;
     
    +uint lastSignature = 0; // 0 is an invalid signature!
     
    +
     // Reset everything between levels
     void particle_init()
     {
    @@ -71,7 +73,14 @@
     // only call from game_shutdown()!!!
     void particle_close()
     {
    -	Particles.clear();
    +	while (!Particles.empty())
    +	{		
    +		particle* part = Particles.back();
    +		part->signature = 0;
    +		delete part;
    +
    +		Particles.pop_back();
    +	}
     }
     
     void particle_page_in()
    @@ -99,43 +108,43 @@
     int Num_particles_hwm = 0;
     
     // Creates a single particle. See the PARTICLE_?? defines for types.
    -particle* particle_create( particle_info *pinfo )
    +particle *particle_create( particle_info *pinfo )
     {
    -	particle new_particle;
    -	int fps = 1;
    -
     	if ( !Particles_enabled )
    +	{
     		return NULL;
    +	}
     
    -	// Init the particle data
    -	memset( &new_particle, 0, sizeof(particle) );
    +	particle* new_particle = new particle();
    +	int fps = 1;
    +	
    +	new_particle->pos = pinfo->pos;
    +	new_particle->velocity = pinfo->vel;
    +	new_particle->age = 0.0f;
    +	new_particle->max_life = pinfo->lifetime;
    +	new_particle->radius = pinfo->rad;
    +	new_particle->type = pinfo->type;
    +	new_particle->optional_data = pinfo->optional_data;
    +	new_particle->tracer_length = pinfo->tracer_length;
    +	new_particle->attached_objnum = pinfo->attached_objnum;
    +	new_particle->attached_sig = pinfo->attached_sig;
    +	new_particle->reverse = pinfo->reverse;
    +	new_particle->particle_index = (int)Particles.size();
     
    -	new_particle.pos = pinfo->pos;
    -	new_particle.velocity = pinfo->vel;
    -	new_particle.age = 0.0f;
    -	new_particle.max_life = pinfo->lifetime;
    -	new_particle.radius = pinfo->rad;
    -	new_particle.type = pinfo->type;
    -	new_particle.optional_data = pinfo->optional_data;
    -	new_particle.tracer_length = pinfo->tracer_length;
    -	new_particle.attached_objnum = pinfo->attached_objnum;
    -	new_particle.attached_sig = pinfo->attached_sig;
    -	new_particle.reverse = pinfo->reverse;
    -	new_particle.particle_index = (int)Particles.size();
    -
     	switch (pinfo->type) {
     		case PARTICLE_BITMAP:
     		case PARTICLE_BITMAP_PERSISTENT: {
     			if (pinfo->optional_data < 0) {
     				Int3();
    +				delete new_particle;
     				return NULL;
     			}
     
    -			bm_get_info( pinfo->optional_data, NULL, NULL, NULL, &new_particle.nframes, &fps );
    +			bm_get_info( pinfo->optional_data, NULL, NULL, NULL, &new_particle->nframes, &fps );
     
    -			if ( new_particle.nframes > 1 )	{
    +			if ( new_particle->nframes > 1 )	{
     				// Recalculate max life for ani's
    -				new_particle.max_life = i2fl(new_particle.nframes) / i2fl(fps);
    +				new_particle->max_life = i2fl(new_particle->nframes) / i2fl(fps);
     			}
     
     			break;
    @@ -143,42 +152,46 @@
     
     		case PARTICLE_FIRE: {
     			if (Anim_bitmap_id_fire < 0) {
    +				delete new_particle;
     				return NULL;
     			}
     
    -			new_particle.optional_data = Anim_bitmap_id_fire;
    -			new_particle.nframes = Anim_num_frames_fire;
    +			new_particle->optional_data = Anim_bitmap_id_fire;
    +			new_particle->nframes = Anim_num_frames_fire;
     
     			break;
     		}
     
     		case PARTICLE_SMOKE: {
     			if (Anim_bitmap_id_smoke < 0) {
    +				delete new_particle;
     				return NULL;
     			}
     
    -			new_particle.optional_data = Anim_bitmap_id_smoke;
    -			new_particle.nframes = Anim_num_frames_smoke;
    +			new_particle->optional_data = Anim_bitmap_id_smoke;
    +			new_particle->nframes = Anim_num_frames_smoke;
     
     			break;
     		}
     
     		case PARTICLE_SMOKE2: {
     			if (Anim_bitmap_id_smoke2 < 0) {
    +				delete new_particle;
     				return NULL;
     			}
     
    -			new_particle.optional_data = Anim_bitmap_id_smoke2;
    -			new_particle.nframes = Anim_num_frames_smoke2;
    +			new_particle->optional_data = Anim_bitmap_id_smoke2;
    +			new_particle->nframes = Anim_num_frames_smoke2;
     
     			break;
     		}
     
     		default:
    -			new_particle.nframes = 1;
    +			new_particle->nframes = 1;
     			break;
     	}
    -
    +	
    +	new_particle->signature = ++lastSignature;
     	Particles.push_back( new_particle );
     
     	// reallocate to additional space if we need it
    @@ -194,10 +207,10 @@
     	}
     #endif
     
    -	return &Particles.back();
    +	return Particles.back();
     }
     
    -particle* particle_create( vec3d *pos, vec3d *vel, float lifetime, float rad, int type, int optional_data, float tracer_length, object *objp, bool reverse )
    +particle *particle_create( vec3d *pos, vec3d *vel, float lifetime, float rad, int type, int optional_data, float tracer_length, object *objp, bool reverse )
     {
     	particle_info pinfo;
     
    @@ -244,17 +257,22 @@
     	if ( Particles.empty() )
     		return;
     
    -	for (SCP_vector<particle>::iterator p = Particles.begin(); p != Particles.end(); ) {	
    -		if (p->age == 0.0f) {
    -			p->age = 0.00001f;
    +	for (SCP_vector<particle*>::iterator p = Particles.begin(); p != Particles.end(); ) {	
    +		particle* part = *p;
    +		if (part->age == 0.0f) {
    +			part->age = 0.00001f;
     		} else {
    -			p->age += frametime;
    +			part->age += frametime;
     		}
     
     		// if it's time expired, remove it
    -		if (p->age > p->max_life) {
    +		if (part->age > part->max_life) {
     			// special case, if max_life is 0 then we want it to render at least once
    -			if ( (p->age > frametime) || (p->max_life > 0.0f) ) {
    +			if ( (part->age > frametime) || (part->max_life > 0.0f) ) {
    +				part->signature = 0;
    +				delete *p;
    +				*p = NULL;
    +
     				*p = Particles.back();
     				Particles.pop_back();
     				continue;
    @@ -262,11 +280,15 @@
     		}
     
     		// if the particle is attached to an object which has become invalid, kill it
    -		if (p->attached_objnum >= 0) {
    +		if (part->attached_objnum >= 0) {
     			// if the signature has changed, or it's bogus, kill it
    -			if ( (p->attached_objnum >= MAX_OBJECTS)
    -				|| (p->attached_sig != Objects[p->attached_objnum].signature) )
    +			if ( (part->attached_objnum >= MAX_OBJECTS)
    +				|| (part->attached_sig != Objects[part->attached_objnum].signature) )
     			{
    +				part->signature = 0;
    +				delete *p;
    +				*p = NULL;
    +
     				*p = Particles.back();
     				Particles.pop_back();
     				continue;
    @@ -274,7 +296,7 @@
     		}
     		// move as a regular particle
     		else {
    -			vm_vec_scale_add2( &p->pos, &p->velocity, frametime );
    +			vm_vec_scale_add2( &part->pos, &part->velocity, frametime );
     		}
     
     		// next particle
    @@ -289,7 +311,13 @@
     	Num_particles = 0;
     	Num_particles_hwm = 0;
     
    -	Particles.clear();
    +	while (!Particles.empty())
    +	{
    +		particle* part = Particles.back();
    +		part->signature = 0;
    +		delete part;
    +		Particles.pop_back();
    +	}
     }
     
     MONITOR( NumParticlesRend )
    @@ -340,15 +368,16 @@
     	if ( Particles.empty() )
     		return;
     
    -	for (SCP_vector<particle>::iterator p = Particles.begin(); p != Particles.end(); ++p) {
    +	for (SCP_vector<particle*>::iterator p = Particles.begin(); p != Particles.end(); ++p) {
    +		particle* part = *p;
     		// skip back-facing particles (ripped from fullneb code)
     		// Wanderer - add support for attached particles
     		vec3d p_pos;
    -		if (p->attached_objnum >= 0) {
    -			vm_vec_unrotate(&p_pos, &p->pos, &Objects[p->attached_objnum].orient);
    -			vm_vec_add2(&p_pos, &Objects[p->attached_objnum].pos);
    +		if (part->attached_objnum >= 0) {
    +			vm_vec_unrotate(&p_pos, &part->pos, &Objects[part->attached_objnum].orient);
    +			vm_vec_add2(&p_pos, &Objects[part->attached_objnum].pos);
     		} else {
    -			p_pos = p->pos;
    +			p_pos = part->pos;
     		}
     
     		if ( vm_vec_dot_to_point(&Eye_matrix.vec.fvec, &Eye_position, &p_pos) <= 0.0f ) {
    @@ -367,11 +396,11 @@
     		rotate = 1;
     
     		// if this is a tracer style particle, calculate tracer vectors
    -		if (p->tracer_length > 0.0f) {			
    +		if (part->tracer_length > 0.0f) {			
     			ts = p_pos;
    -			temp = p->velocity;
    +			temp = part->velocity;
     			vm_vec_normalize_quick(&temp);
    -			vm_vec_scale_add(&te, &ts, &temp, p->tracer_length);
    +			vm_vec_scale_add(&te, &ts, &temp, part->tracer_length);
     
     			// don't bother rotating
     			rotate = 0;
    @@ -390,33 +419,33 @@
     		}
     
     		// pct complete for the particle
    -		pct_complete = p->age / p->max_life;
    +		pct_complete = part->age / part->max_life;
     
     		// figure out which frame we should be using
    -		if (p->nframes > 1) {
    -			framenum = fl2i(pct_complete * p->nframes + 0.5);
    -			CLAMP(framenum, 0, p->nframes-1);
    +		if (part->nframes > 1) {
    +			framenum = fl2i(pct_complete * part->nframes + 0.5);
    +			CLAMP(framenum, 0, part->nframes-1);
     
    -			cur_frame = p->reverse ? (p->nframes - framenum - 1) : framenum;
    +			cur_frame = part->reverse ? (part->nframes - framenum - 1) : framenum;
     		} else {
     			cur_frame = 0;
     		}
     
    -		if (p->type == PARTICLE_DEBUG) {
    +		if (part->type == PARTICLE_DEBUG) {
     			gr_set_color( 255, 0, 0 );
    -			g3_draw_sphere_ez( &p_pos, p->radius );
    +			g3_draw_sphere_ez( &p_pos, part->radius );
     		} else {
    -			framenum = p->optional_data;
    +			framenum = part->optional_data;
     
    -			Assert( cur_frame < p->nframes );
    +			Assert( cur_frame < part->nframes );
     
     			// if this is a tracer style particle
    -			if (p->tracer_length > 0.0f) {
    -				batch_add_laser( framenum + cur_frame, &ts, p->radius, &te, p->radius );
    +			if (part->tracer_length > 0.0f) {
    +				batch_add_laser( framenum + cur_frame, &ts, part->radius, &te, part->radius );
     			}
     			// draw as a regular bitmap
     			else {
    -				batch_add_bitmap( framenum + cur_frame, tmap_flags, &pos, p->particle_index % 8, p->radius, alpha );
    +				batch_add_bitmap( framenum + cur_frame, tmap_flags, &pos, part->particle_index % 8, part->radius, alpha );
     			}
     
     			render_batch = true;
    @@ -517,3 +546,4 @@
     		particle_create( &pe->pos, &tmp_vel, life, radius, type, optional_data );
     	}
     }
    +
    Index: code/particle/particle.h
    ===================================================================
    --- code/particle/particle.h	(revision 7100)
    +++ code/particle/particle.h	(working copy)
    @@ -90,13 +90,14 @@
     	int		attached_sig;		// to check for dead/nonexistent objects
     	ubyte	reverse;			// play any animations in reverse
     	int		particle_index;		// used to keep particle offset in dynamic array for orient usage
    +
    +	uint signature;
     } particle;
     
     // Creates a single particle. See the PARTICLE_?? defines for types.
    -particle* particle_create( particle_info *pinfo );
    -particle* particle_create( vec3d *pos, vec3d *vel, float lifetime, float rad, int type, int optional_data = -1, float tracer_length=-1.0f, struct object *objp=NULL, bool reverse=false );
    +particle *particle_create( particle_info *pinfo );
    +particle *particle_create( vec3d *pos, vec3d *vel, float lifetime, float rad, int type, int optional_data = -1, float tracer_length=-1.0f, struct object *objp=NULL, bool reverse=false );
     
    -
     //============================================================================
     //============== HIGH-LEVEL PARTICLE SYSTEM CREATION CODE ====================
     //============================================================================
    
    patch file icon particleFix_r7099.patch (14,133 bytes) 2011-03-31 11:30 +

-Relationships
+Relationships

-Notes

~0012653

Echelon9 (developer)

Is this still required? See http://www.hard-light.net/forums/index.php?topic=44821.msg1491033#msg1491033

~0012654

m_m (developer)

It needs a review to check that I didn't break anything.

~0012702

The_E (administrator)

Added to trunk in rev 7224
+Notes

-Issue History
Date Modified Username Field Change
2011-03-31 11:30 m_m New Issue
2011-03-31 11:30 m_m File Added: particleFix_r7099.patch
2011-04-10 08:51 Echelon9 Note Added: 0012653
2011-04-10 08:51 Echelon9 Status new => feedback
2011-04-10 09:03 m_m Note Added: 0012654
2011-06-01 15:41 The_E Note Added: 0012702
2011-06-01 15:41 The_E Status feedback => resolved
2011-06-01 15:41 The_E Fixed in Version => 3.6.13
2011-06-01 15:41 The_E Resolution open => fixed
2011-06-01 15:41 The_E Assigned To => The_E
+Issue History