Segmentation fault in SoGLLazyElement::endCaching

[wwmayer]

Hi Coin team,

under certain circumstances it can happen that a segmentation fault occurs in the function SoGLLazyElement::endCaching() at the line

*elem->postcachestate = elem->glstate;

because the postcachestate variable has been nullified in a previous call of SoGLLazyElement::endCaching().

The crash occurred in FreeCAD where some custom Inventor classes are involved but I was able to implement a little demo application using only Coin3D node types.

Here is the code of the demo:

#ifdef WIN32
# include <Windows.h>
#endif
#include <GL/gl.h>
#include <Inventor/SoDB.h>
#include <Inventor/SoInteraction.h>
#include <Inventor/actions/SoGLRenderAction.h>
#include <Inventor/actions/SoSearchAction.h>
#include <Inventor/draggers/SoCenterballDragger.h>
#include <Inventor/nodes/SoAnnotation.h>
#include <Inventor/nodes/SoMaterial.h>
#include <Inventor/nodes/SoSeparator.h>

#include <Inventor/Qt/SoQt.h>
#include <Inventor/Qt/viewers/SoQtExaminerViewer.h>

class MyViewer : public SoQtExaminerViewer {
public:
    MyViewer(QWidget* parent, SoNode* anno)
        : SoQtExaminerViewer(parent)
        , anno(anno)
    {
    }
    // Use this function to add an Annotation node after the
    // scene has been rendered
    virtual void setViewing(SbBool enable) {
        SoNode* root = getSceneGraph();

        SoSearchAction searchAction;
        searchAction.setType(SoSeparator::getClassTypeId());
        searchAction.setInterest(SoSearchAction::FIRST);
        searchAction.setName("GroupOnTopPreSel");
        searchAction.apply(root);
        SoPath* selectionPath = searchAction.getPath();

        if (selectionPath) {
            SoSeparator* sep = static_cast<SoSeparator*>(selectionPath->getTail());
            sep->addChild(anno);
        }
    }

private:
    SoNode* anno;
};

int main(int argc, char** argv)
{
    // built with:
    // g++ cache.cpp -fPIC -I/usr/include -I/usr/include/x86_64-linux-gnu/qt5  
    // -I/usr/include/x86_64-linux-gnu/qt5/QtCore
    // -L/usr/lib/x86_64-linux-gnu -lCoin -lSoQt -lGL -lQt5Core
    //

    SoDB::init();
    SoInteraction::init();

    SoSeparator* root = new SoSeparator();
    root->ref();

    SoMaterial* mat = new SoMaterial();
    mat->transparency = 0.5f;
    mat->diffuseColor.setIgnored(true);
    mat->setOverride(true);

    SoSeparator* edit = new SoSeparator();
    edit->setName("GroupOnTopPreSel");
    edit->addChild(mat);
    root->addChild(edit);

    SoSeparator* view = new SoSeparator();
    view->renderCaching.setValue(SoSeparator::AUTO);
    view->addChild(new SoCenterballDragger());

    root->addChild(view);

    // An Annotation node must be used here because it delays the rendering
    SoSeparator* anno = new SoAnnotation();
    anno->ref();
    anno->addChild(view);

    QWidget* mainwin = SoQt::init(argc, argv, argv[0]);
    MyViewer* eviewer = new MyViewer(mainwin, anno);

    // Transparency type must be set to SORTED_OBJECT_SORTED_TRIANGLE_BLEND in order
    // to activate the caching mechanism
    SoGLRenderAction* glAction = eviewer->getGLRenderAction();
    glAction->setTransparencyType(SoGLRenderAction::SORTED_OBJECT_SORTED_TRIANGLE_BLEND);
    eviewer->setSceneGraph(root);

    eviewer->show();

    SoQt::show(mainwin);
    SoQt::mainLoop();

    delete eviewer;
    root->unref();
    anno->unref();
    SoQt::done();
    return 0;
}

So far the crash only seems to occur if all of the conditions below are fulfilled:

• the scene graph contains an SoCenterballDragger (I don't know if it crashes with other node types but I only have seen it with this one)
• an SoAnnotation node with the above SoCenterballDragger is added to the scene graph at runtime after it has been rendered
• the transparency type of the render action must be set to SORTED_OBJECT_SORTED_TRIANGLE_BLEND (or SORTED_OBJECT_SORTED_TRIANGLE_ADD)
• the transparency of an SoMaterial node is set

The steps to reproduce:

• Compile the code and start the application
• click in the view and rotate the model
• click on the arrow button on the right side to make sure MyViewer::setViewing() is invoked

After the SoAnnotation has been added to the scene graph the functions SoGLLazyElement::beginCaching() and SoGLLazyElement::endCaching() will be called a few times but then there is a nested call of SoGLLazyElement::beginCaching() where the function getInstance() returns two times the same instance of SoGLLazyElement.
Afterwards SoGLLazyElement::endCaching() will be called twice and inside the first call precachestate and postcachestate are nullified. In the second call postcachestate will be dereferenced and thus causes a segmentation fault because it's NULL.

For more details have a look at: https://forum.freecadweb.org/viewtopic.php?f=18&t=43305&start=10#p412537

Btw:
The crash can be reproduced with the current master of Coin3D and SoQt.
The crash can be reproduced on Windows and Linux and probably on any other OS

[VolkerEnderlein]

Thanks for the thorough issue report, it is highly appreciated.
I was able to reproduce the faulty behavior, but need some time to come up with a proper fix.

[YvesBoyadjian]

I think I spotted the problem :

• The method "SoGLLazyElement::beginCaching()" is called two times :
• First by "SoGLCacheList::open()"
• Then by "SoPrimitiveVertexCache::SoPrimitiveVertexCache()"
• Logically, the method "SoGLLazyElement::endCaching()" is also called two times :
• First by "SoPrimitiveVertexCache::close()"
• Then by "SoGLCacheList::close()"
  The problem is that these method have not been designed for nested calls, especially for the variables "postcachestate" and "precachestate"

So using some stacks for those two variables seems to do the trick :

• In SoGLLazyElement.h, instead of this :

  GLState * postcachestate;
  GLState * precachestate;

• I put this :

  SbPList postcachestateList;
  SbPList precachestateList;
  GLState * postcachestate() const {
	  int len = postcachestateList.getLength();
	  if (len == 0) {
		  return nullptr;
	  }
	  return (GLState*)postcachestateList.get(len - 1);
  }
  GLState * precachestate() const {
	  int len = precachestateList.getLength();
	  if (len == 0) {
		  return nullptr;
	  }
	  return (GLState*)precachestateList.get(len - 1);
  }

• And in cpp :

void
SoGLLazyElement::push(SoState * stateptr)
{
  inherited::push(stateptr);
  SoGLLazyElement * prev = (SoGLLazyElement*) this->getNextInStack();
  this->state = stateptr; // needed to send GL texture
  this->glstate = prev->glstate;
  this->colorindex = prev->colorindex;
  this->transpmask = prev->transpmask;
  this->colorpacker = prev->colorpacker;
  this->precachestateList = prev->precachestateList;
  this->postcachestateList = prev->postcachestateList;
  this->didsetbitmask = prev->didsetbitmask;
  this->didntsetbitmask = prev->didntsetbitmask;
  this->cachebitmask = prev->cachebitmask;
  this->opencacheflags = prev->opencacheflags;
}

void
SoGLLazyElement::beginCaching(SoState * state, GLState * prestate,
                              GLState * poststate)
{
  SoGLLazyElement * elem = getInstance(state);

  elem->send(state, ALL_MASK); // send lazy state before starting to build cache
  *prestate = elem->glstate; // copy current GL state
  prestate->diffusenodeid = elem->coinstate.diffusenodeid;
  prestate->transpnodeid = elem->coinstate.transpnodeid;
  elem->precachestateList.append(prestate);
  elem->postcachestateList.append(poststate);
  elem->precachestate()->cachebitmask = 0;
  elem->postcachestate()->cachebitmask = 0;
  elem->didsetbitmask = 0;
  elem->didntsetbitmask = 0;
  elem->cachebitmask = 0;
  elem->opencacheflags = 0;
}

void
SoGLLazyElement::endCaching(SoState * state)
{
  SoGLLazyElement * elem = getInstance(state);

  *elem->postcachestate() = elem->glstate;
  elem->postcachestate()->cachebitmask = elem->cachebitmask;
  elem->precachestate()->cachebitmask = elem->didntsetbitmask;

  // unset diffuse mask since it's used by the dependency test
  elem->precachestate()->cachebitmask &= ~DIFFUSE_MASK;

  // set diffuse mask if this cache depends on a material outside the
  // cache.
  if (elem->opencacheflags & FLAG_DIFFUSE_DEPENDENCY) {
    elem->precachestate()->cachebitmask |= DIFFUSE_MASK;
  }

  elem->precachestateList.remove(elem->precachestateList.getLength()-1);
  elem->postcachestateList.remove(elem->postcachestateList.getLength() - 1);
  elem->opencacheflags = 0;
}

And using the method precachestate() instead of the variable in "SoGLLazyElement::send()"

I tested this solution, and it doesn't crash anymore.

Another even simpler solution could be for SoPrimitiveVertexCache constructor not to call SoGLLazyElement::beginCaching() if someone has already called it before.
Though I have not tested this last one.

[luzpaz]

any traction on this?

[VolkerEnderlein]

I had a closer look onto this but I am not convinced the proposed solution is the right one. The SoPrimitiveVertexCache should IMHO not setup a new cache but rather use the existing. Will need to dive further into the code.