NULL pointer access in HlmsPbs::preparePassHash()

[Chungzuwalla]

System Information

• Ogre Version: 2.1unstable
• Operating System / Platform: Win7 x64, Visual Studio 2015
• RenderSystem: Direct3D11

Detailled description

Note about this bug report: I'm using 2.1unstable (out of necessity, not my choice) which is pretty old, so the bug I describe may have been fixed already. Also, I don't completely understand the Python-Ogre codebase I have inherited, and it may use Ogre in strange unintended ways. So, this crash may be a consequence of bad design of my codebase rather than Ogre itself.

When enabling a directional light, I get a NULL pointer dereference in HlmsPbs::preparePassHash(), around line 1199 of OgreHlmsPbs.cpp:

  if( i >= shadowCastingDirLights && i < numDirectionalLights )
  {
      while( affectedLights[nonShadowLightIdx] )  <---- CRASH
          ++nonShadowLightIdx;
      light = globalLightList.lights[nonShadowLightIdx++];
      assert( light->getType() == Light::LT_DIRECTIONAL );
  }

affectedLights refers to a std::vector<bool> which is the member variable mAffectedLights of a CompositorShadowNode. mAffectedLights is empty, so affectedLights[nonShadowLightIdx] is out of range.

mAffectedLights is not iinitialized on CompositorShadowNode construction, so it is empty by default. The only code that adds entries to mAffectedLights is the method CompositorShadowNode::clearShadowCastingLights() which ensures mAffectedLights always contains at least one entry, with value "false":

   mAffectedLights.clear();
   //Reserve last place for avoid crashing with static
   //lights that weren't collected into globalLightList
   mAffectedLights.resize( globalLightList.lights.size() + 1u, false );

CompositorShadowNode::clearShadowCastingLights() does not get called before the crash occurs. So I think either:

• CompositorShadowNode::clearShadowCastingLights() should be called before HlmsPbs::preparePassHash(), or
• the CompositorShadowNode constructor should add one entry "false" to mAffectedLights, or
• the code in HlmsPbs::preparePassHash() should guard against mAffectedLights being empty.

I fixed the crash by adding this line to the end of the CompositorShadowNode constructor:

   mAffectedLights.resize( 1u, false );

I don't know if this is correct.

[darksylinc]

Is there a small/quick way to repro this? (you could try exporting the scene via SceneFormatExporter and sending it to me)

I would like to understand what's going on, because that path on preparePassHash should only be called if there is a valid shaodow node, and if so, then CompositorShadowNode::_update should be getting called first (by CompositorPassScene::execute), which in turns calls CompositorShadowNode::buildClosestLightList and in turn calls clearShadowCastingLights. Thus the crash should be impossible.

The only two things that come to mind that could cause this crash:

1. buildClosestLightList starts with this:

if( mLastCamera == newCamera && mLastFrame == currentFrameCount )
{
        return;
}

Perhaps this is getting into the return, for some odd reason (should not happen for the first frame... this code prevents is here to prevent calling buildClosestLightList twice in a row and reuse previous results)

2. CompositorPassScene::mUpdateShadowNode is false, due to mDefinition->mShadowNodeRecalculation == SHADOW_NODE_RECALCULATE failing this means the compositor you provided contains a "shadows reuse" in the declaration for the first time the shadow node is used (which would be wrong, as even if we guard about this edge case, the rendering will be wrong). Or alternatively, pass->_setUpdateShadowNode( true ); in CompositorWorkspace::setupPassesShadowNodes never got called (or got called too late), which would likely suggest an Ogre bug.

[Chungzuwalla]

Thanks darksylinc for following this up.

SceneFormatExporter aka SceneFormat is not in our Ogre 2.1unstable, it must be too old. It could be a nightmare anyway as we are using many digital assets licensed from 3rd parties.

I've been cutting stuff out of our system to create a minimal example, and I am getting a clearer idea of what's going on. I'm also learning a bit more about Ogre internals which is useful for me, so I hope you can bear with me. It's not case 1, because CompositorShadowNode::_update() does not get called at all before the crash. It is something like case 2.

There is a log below, after I added some debug printfs to Ogre. It looks like the workspace contains 1 node with 13 passes, of which 5 use a shadowNode, and they are all the same shadowNode. setupPassesShadowNodes() sets mUpdateShadowNode TRUE on the first of those 5 passes, and FALSE for the rest. I guess that is because it expects to execute the passes in that order. But in the call to renderOneFrame(), the pass with mUpdateShadowNode TRUE (pass 055A9F00) is not executed, because (executionMask & passDef->mExecutionMask) is 0. Later a different pass is executed which uses the shadowNode, but has mUpdateShadowNode 0, so it does not call CompositorShadowNode::_update(). So it crashes (after printing the last line of this log). That's about all I know so far.

 CompositorWorkspace[this=0508CDB0]::CompositorWorkspace()
     CompositorPassScene[this=055A9F00]::CompositorPassScene(): mDefinition->mShadowNode != IdString()
       mDefinition is 039D8100
       CompositorShadowNode[this=0571DF90]::CompositorShadowNode()
         CompositorWorkspace[this=0508CDB0]::findOrCreateShadowNode(): Created CompositorShadowNode() 0571DF90
       mShadowNode set to 0571DF90
       mDefinition->mShadowNodeRecalculation is 2
     CompositorPassScene[this=2C7492D0]::CompositorPassScene(): mDefinition->mShadowNode != IdString()
       mDefinition is 039D8D00
       mShadowNode set to 0571DF90
       mDefinition->mShadowNodeRecalculation is 2
     CompositorPassScene[this=2C7491F0]::CompositorPassScene(): mDefinition->mShadowNode != IdString()
       mDefinition is 039D9600
       mShadowNode set to 0571DF90
       mDefinition->mShadowNodeRecalculation is 2
     CompositorPassScene[this=2C749110]::CompositorPassScene(): mDefinition->mShadowNode != IdString()
       mDefinition is 039D9700
       mShadowNode set to 0571DF90
       mDefinition->mShadowNodeRecalculation is 2
     CompositorPassScene[this=2C749FF0]::CompositorPassScene(): mDefinition->mShadowNode != IdString()
       mDefinition is 039D9800
       mShadowNode set to 0571DF90
       mDefinition->mShadowNodeRecalculation is 2
   CompositorWorkspace[this=0508CDB0]::setupPassesShadowNodes()
     node 052A6030, pass 05824F00:
     node 052A6030, pass 055A9FE0:
       shadowNode is 0571DF90, pass->getShadowNode() is 00000000, different
     node 052A6030, pass 055A9F00:
       shadowNode is 0571DF90, pass->getShadowNode() is 0571DF90, SAME
         recalc is 2
         lastCamera 00000000, pass->getCamera() is 0571E520
           CompositorPassScene[this=055A9F00]::_setUpdateShadowNode(): mUpdateShadowNode set to 1
     node 052A6030, pass 04F7BD60:
     node 052A6030, pass 04F7BC90:
     node 052A6030, pass 04F7BBC0:
     node 052A6030, pass 2CAA2F10:
     node 052A6030, pass 2CFFEEC0:
     node 052A6030, pass 2C7492D0:
       shadowNode is 0571DF90, pass->getShadowNode() is 0571DF90, SAME
         recalc is 2
         lastCamera 0571E520, pass->getCamera() is 0571E520
           CompositorPassScene[this=2C7492D0]::_setUpdateShadowNode(): mUpdateShadowNode set to 0
     node 052A6030, pass 2C7491F0:
       shadowNode is 0571DF90, pass->getShadowNode() is 0571DF90, SAME
         recalc is 2
         lastCamera 0571E520, pass->getCamera() is 0571E520
           CompositorPassScene[this=2C7491F0]::_setUpdateShadowNode(): mUpdateShadowNode set to 0
     node 052A6030, pass 2C749110:
       shadowNode is 0571DF90, pass->getShadowNode() is 0571DF90, SAME
         recalc is 2
         lastCamera 0571E520, pass->getCamera() is 0571E520
           CompositorPassScene[this=2C749110]::_setUpdateShadowNode(): mUpdateShadowNode set to 0
     node 052A6030, pass 2C749FF0:
       shadowNode is 0571DF90, pass->getShadowNode() is 0571DF90, SAME
         recalc is 2
         lastCamera 0571E520, pass->getCamera() is 0571E520
           CompositorPassScene[this=2C749FF0]::_setUpdateShadowNode(): mUpdateShadowNode set to 0
     node 052A6030, pass 2C74A0D0:
       shadowNode is 0571DF90, pass->getShadowNode() is 00000000, different
 Root[this=03D75640]::renderOneFrame()
   CompositorManager2[this=05146E80]::_update(): 1 workspaces
     CompositorWorkspace[this=0508CDB0]::_update(): 1 nodes
       CompositorNode[this=052A6030]::_update(): 13 passes
        executionMask: 07
        shadowNode is 00000000
        pass 05824F00:
         passDef->mExecutionMask: C0
         executionMask & passDef->mExecutionMask: 00
         shadowNode is 00000000
         DON'T execute pass
        pass 055A9FE0:
         is a PASS_SCENE, pass->mShadowNode is 00000000, pass->mUpdateShadowNode is 0
         passDef->mExecutionMask: 40
         executionMask & passDef->mExecutionMask: 00
         shadowNode is 00000000
         DON'T execute pass
        pass 055A9F00:
         is a PASS_SCENE, pass->mShadowNode is 0571DF90, pass->mUpdateShadowNode is 1
         passDef->mExecutionMask: 80
         executionMask & passDef->mExecutionMask: 00
         shadowNode is 00000000
         DON'T execute pass
        pass 04F7BD60:
         passDef->mExecutionMask: FF
         executionMask & passDef->mExecutionMask: 07
         shadowNode is 00000000
         execute pass...
        pass 04F7BC90:
         passDef->mExecutionMask: FF
         executionMask & passDef->mExecutionMask: 07
         shadowNode is 00000000
         execute pass...
        pass 04F7BBC0:
         passDef->mExecutionMask: FF
         executionMask & passDef->mExecutionMask: 07
         shadowNode is 00000000
         execute pass...
        pass 2CAA2F10:
         passDef->mExecutionMask: 02
         executionMask & passDef->mExecutionMask: 02
         shadowNode is 00000000
         execute pass...
        pass 2CFFEEC0:
         passDef->mExecutionMask: C0
         executionMask & passDef->mExecutionMask: 00
         shadowNode is 00000000
         DON'T execute pass
        pass 2C7492D0:
         is a PASS_SCENE, pass->mShadowNode is 0571DF90, pass->mUpdateShadowNode is 0
         passDef->mExecutionMask: 01
         executionMask & passDef->mExecutionMask: 01
         shadowNode is 00000000
         execute pass...
         CompositorPassScene[this=2C7492D0]::execute()
           mShadowNode is 0571DF90, shadowNode set to 0571DF90
           mDefinition->mShadowNodeRecalculation is 2
           mUpdateShadowNode is 0

[Chungzuwalla]

I come to the conclusion that the workspaces in my project were set up by someone who didn't really understand executionMasks and vpModifierMasks. There are comments like "vpModifierMask is used to control which compositor passes are active", and values calculated as executionMask are passed as vpModifierMask.

It may be just good luck that the project worked in the first place.

I don't know yet whether Ogre can, or should, protect itself from crashing when someone passing almost random values for these flags. I'll keep looking till I can decide if there really is a problem in Ogre. So far I think it is just "user error".

[Chungzuwalla]

@darksylinc I've found a fix that works for me, and I think there might be an Ogre bug.

CompositorWorkspace::setupPassesShadowNodes() finds all the passes that use each shadowNode, and sets mUpdateShadowNode TRUE on the first of those passes, and FALSE for the rest. But it does not check executionFlags to see whether the pass with mUpdateShadowNode TRUE will actually execute. So one of the later passes can use the uninitialized shadowNode.

I changed the code around line 420 of OgreCompositorWorkspace.cpp from this:

                        else if( recalc == SHADOW_NODE_FIRST_ONLY )
                        {
                            if( lastCamera != pass->getCamera() )
                            {

to this:

                        else if( recalc == SHADOW_NODE_FIRST_ONLY )
                        {
                            if( lastCamera != pass->getCamera()
                                &&
                                (mExecutionMask & pass->getDefinition()->mExecutionMask))
                            {

The crash went away, and I get shadows that seem correct.

The current Ogre 2.1 codebase seems to have the same issue, but obviously I can't run it so there may be a fix elsewhere.

Hope this helps!