View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002421 | FSSCP | scripting | public | 2011-03-31 15:30 | 2011-06-01 19:41 |
Reporter | m_m | Assigned To | The_E | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.6.13 | ||||
Fixed in Version | 3.6.13 | ||||
Summary | 0002421: Pointer to a particle returned by particle_create() becomes invalid | ||||
Description | When 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 Information | I compiled and tested the code using Visual Studio 2010. | ||||
Tags | No tags attached. | ||||
2011-03-31 15:30
|
particleFix_r7099.patch (14,133 bytes)
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 ==================== //============================================================================ |
|
Is this still required? See http://www.hard-light.net/forums/index.php?topic=44821.msg1491033#msg1491033 |
|
It needs a review to check that I didn't break anything. |
|
Added to trunk in rev 7224 |
Date Modified | Username | Field | Change |
---|---|---|---|
2011-03-31 15:30 | m_m | New Issue | |
2011-03-31 15:30 | m_m | File Added: particleFix_r7099.patch | |
2011-04-10 12:51 | Echelon9 | Note Added: 0012653 | |
2011-04-10 12:51 | Echelon9 | Status | new => feedback |
2011-04-10 13:03 | m_m | Note Added: 0012654 | |
2011-06-01 19:41 | The_E | Note Added: 0012702 | |
2011-06-01 19:41 | The_E | Status | feedback => resolved |
2011-06-01 19:41 | The_E | Fixed in Version | => 3.6.13 |
2011-06-01 19:41 | The_E | Resolution | open => fixed |
2011-06-01 19:41 | The_E | Assigned To | => The_E |