Crash in lightattrib.cxx + fix

Hi,

I’ve encountered a crashbug in Panda3D. To reproduce it, run the following script:

import direct.directbase.DirectStart
from pandac.PandaModules import AmbientLight, DirectionalLight

for i in xrange(9):
    render.setLight(render.attachNewNode(AmbientLight( "ambientLight%d" % i).upcastToPandaNode()))
    
model = loader.loadModel("smiley.egg")
model.reparentTo(render)
model.setPos(0,10,0)

taskMgr.step()
taskMgr.step()

I’m not 100% sure if the crash occurs in the official Panda releases as well. My Panda3D version has been built with VS 2005 (I compile it myself) and the crash occurs because of an invalid range in a std::vector in the lighting code. Since the STL implementation has changed between VS 2003 and VS 2005, the official pre-built version might not be affected. But anyhow, here’s why it crashes:

in panda\src\pgraph\lightattrib.cxx there’s the function “filter_to_max”:

////////////////////////////////////////////////////////////////////
//     Function: LightAttrib::filter_to_max
//       Access: Public
//  Description: Returns a new LightAttrib, very much like this one,
//               but with the number of on_lights reduced to be no
//               more than max_lights.  The number of off_lights in
//               the new LightAttrib is undefined.
//
//               The number of AmbientLights is not included in the
//               count.  All AmbientLights in the original attrib are
//               always included in the result, regardless of the
//               value of max_lights.
////////////////////////////////////////////////////////////////////
CPT(LightAttrib) LightAttrib::
filter_to_max(int max_lights) const {
  if (max_lights < 0 || (int)_on_lights.size() <= max_lights) {
    // Trivial case: this LightAttrib qualifies.
    return this;
  }

  // Since check_filtered() will clear the _filtered list if we are out
  // of date, we should call it first.
  check_filtered();

  Filtered::const_iterator fi;
  fi = _filtered.find(max_lights);
  if (fi != _filtered.end()) {
    // Easy case: we have already computed this for this particular
    // LightAttrib.
    return (*fi).second;
  }

  // Harder case: we have to compute it now.  We must choose the n
  // lights with the highest priority in our list of lights.
  Lights priority_lights, ambient_lights;

  // Separate the list of lights into ambient lights and other lights.
  Lights::const_iterator li;
  for (li = _on_lights.begin(); li != _on_lights.end(); ++li) {
    const NodePath &np = (*li);
    nassertr(!np.is_empty() && np.node()->as_light() != (Light *)NULL, this);
    if (np.node()->is_exact_type(AmbientLight::get_class_type())) {
      ambient_lights.push_back(np);
    } else {
      priority_lights.push_back(np);
    }
  }

  // This sort function uses the STL function object defined above.
  sort(priority_lights.begin(), priority_lights.end(), 
       CompareLightPriorities());

  // Now lop off all of the lights after the first max_lights.
  priority_lights.erase(priority_lights.begin() + max_lights, priority_lights.end());

  // Put the ambient lights back into the list.
  for (li = ambient_lights.begin(); li != ambient_lights.end(); ++li) {
    priority_lights.push_back(*li);
  }

  // And re-sort the ov_set into its proper order.
  priority_lights.sort();

  // Now create a new attrib reflecting these lights.
  PT(LightAttrib) attrib = new LightAttrib;
  attrib->_on_lights.swap(priority_lights);

  CPT(RenderAttrib) new_attrib = return_new(attrib);

  // Finally, record this newly-created attrib in the map for next
  // time.
  CPT(LightAttrib) light_attrib = (const LightAttrib *)new_attrib.p();
  ((LightAttrib *)this)->_filtered[max_lights] = light_attrib;
  return light_attrib;
}

The problem is, that this code can produce an invalid range:

  // Now lop off all of the lights after the first max_lights.
  priority_lights.erase(priority_lights.begin() + max_lights, priority_lights.end());

As soon as the priority_lights vector is smaller than max_lights, the range (begin+max_lights, end) becomes invalid because begin+max_lights is beyond the end already.

This problem might not be appearent in the VS 2003 STL, but it cannot be healthy to have an invalid range in panda.

So here’s the fix:

Index: lightAttrib.cxx
===================================================================
RCS file: /cvsroot/panda3d/panda/src/pgraph/lightAttrib.cxx,v
retrieving revision 1.14
diff -r1.14 lightAttrib.cxx
525,526c525,528
<   priority_lights.erase(priority_lights.begin() + max_lights,
<                         priority_lights.end());
---
>   if (priority_lights.size() > max_lights) {
>       priority_lights.erase(priority_lights.begin() + max_lights,
>                             priority_lights.end());
>   }

Please apply this patch to the sources if you see it fit.

Cheers,

Erik

Confirmed a segfault on the second step() call indeed on Ubuntu Gutsy Gibbon 7.10, official 1.4.2 debian package.

Fix applied, with our enthusiastic thanks.

Note, by the way, that upcastToPandaNode() hasn’t been necessary since Panda3D version 1.1.

David