Page 1 of 1

Need help to fix UB which leads to crash

Posted: Sat Oct 11, 2014 7:53 pm
by crousser
I draw your attention to this problem and I hope that in the coming updates, it will be fixed, for example in such a way

Code: Select all

 Pass& Pass::operator=(const Pass& oth)
    {
       mName = oth.mName;
       // mHash = oth.mHash;              // <---------- it should be removed or commented out
       mAmbient = oth.mAmbient;
//
//  ....
//

       _dirtyHash();                       

       return *this;
    }
Why? The answer can be found here
Because one of the reasons assertions in QueuedRenderableCollection::addRenderable and QueuedRenderableCollection::merge (in debug mode with message ""Error inserting new pass entry into PassGroupRenderableMap""), and one of the reasons crashes in release (when Ogre user tries assignment passes manualy use operator=). Also a see the use of potentially dangerous assignment passes within OgreCompositorInstance and SSAO exaple

Re: Need help to fix UB which leads to crash

Posted: Sun Oct 12, 2014 10:24 am
by spacegaier
Can you please create a ticket on JIRA for it and include if possible the minimal coding example required to reproduce the issue? Thank you.

Also was does "UB" stand for in your context?

Re: Need help to fix UB which leads to crash

Posted: Mon Oct 13, 2014 9:29 am
by mmixLinus

Re: Need help to fix UB which leads to crash

Posted: Mon Oct 13, 2014 5:10 pm
by crousser
Hi everyone =)

I will try to explain what I meant when I talked about UB

Read this:
For set and multiset the value type is the same as the key type. For map and multimap it is equal to pair<const Key, T>. Keys in an associative container are immutable.
I hope everyone understands what it means. If you violate this requirement, we obtain inconsistent(not sure of the exact term? I hope you will understand =) ) container std :: map. For these purposes are used in Ogre container with "dirty pass-keys" - Ogre::Pass::msDirtyHashList.
Why I say about UB? Because, in an inconsistent associative container, surprisingly, some of the methods will "work fine" (without causing a crash, but will not do what you expect), like:
.erase()
.find()
.insert()
.clear()

But, Iteration from beginning to end on such a container - almost 100% force crashed you app (that's what makes QueuedRenderableCollection::merge()).

Here's an example, uncoordinated container after a manual assignment phase in user code (i add some log in Ogre ):

Code: Select all

QueuedRenderableCollection::addRenderable() - Insert : hash = 493633536 , name: "Emissive/Lamps", pointer:0x4e8e6b98 {mParent=0x4e881388 {mPasses={ size=2 } mIlluminationPasses={ size=0 } mParent=0x50063d58 {...} ...} ...}
QueuedRenderableCollection::addRenderable() - Insert : hash = 493633536 , name: "Emissive/Lamps", pointer:0x5006bd20 {mParent=0x50063838 {mPasses={ size=2 } mIlluminationPasses={ size=0 } mParent=0x50039b60 {...} ...} ...}
QueuedRenderableCollection::addRenderable() - Insert : hash = 497352704 , name: "Emissive/Lamps", pointer:0x4e910038 {mParent=0x4e8ab840 {mPasses={ size=2 } mIlluminationPasses={ size=0 } mParent=0x4e8ff5d8 {...} ...} ...}
Function: Ogre::Viewport::update(void), for: "rtt/103521320/RT_1_0"
Function: Ogre::SceneManager::_renderScene(Ogre::Camera *, Ogre::Viewport *, bool)
updateAllControllers() - finish
call prepareRenderQueue()
Function: Ogre::Viewport::update(void), for: "game RT-main"
Function: Ogre::SceneManager::_renderScene(Ogre::Camera *, Ogre::Viewport *, bool)
updateAllControllers() - finish
call prepareRenderQueue()
QueuedRenderableCollection::addRenderable() - Insert : hash = 497352704 , name: "Emissive/Lamps", pointer:0x4e8e6b98 {mParent=0x4e881388 {mPasses={ size=2 } mIlluminationPasses={ size=0 } mParent=0x50063d58 {...} ...} ...}
QueuedRenderableCollection::addRenderable() - Insert : hash = 493633536 , name: "Emissive/Lamps", pointer:0x5006bd20 {mParent=0x50063838 {mPasses={ size=2 } mIlluminationPasses={ size=0 } mParent=0x50039b60 {...} ...} ...}
Debug Error!
"Error inserting new pass entry into PassGroupRenderableMap"
and here a part of mGrouped data at the assertion time (the most interesting is in the range [487 -489])

Code: Select all

//....
+		[485]	(0x54309ca0 {mParent=0x54309680 {mPasses={ size=4 } mIlluminationPasses={ size=0 } mParent=0x54309478 {...} ...} ...}, 0x52790d80 { size=0 })	std::pair<Ogre::Pass * const,std::vector<Ogre::Renderable *,Ogre::STLAllocator<Ogre::Renderable *,Ogre::CategorisedAllocPolicy<0> > > *>
+		[486]	(0x5430af00 {mParent=0x5430a8e0 {mPasses={ size=4 } mIlluminationPasses={ size=0 } mParent=0x5430a6d8 {...} ...} ...}, 0x52790da8 { size=0 })	std::pair<Ogre::Pass * const,std::vector<Ogre::Renderable *,Ogre::STLAllocator<Ogre::Renderable *,Ogre::CategorisedAllocPolicy<0> > > *>
+		[487]	(0x4e8e6b98 {mParent=0x4e881388 {mPasses={ size=2 } mIlluminationPasses={ size=0 } mParent=0x50063d58 {...} ...} ...}, 0x4fb262c0 { size=0 })	std::pair<Ogre::Pass * const,std::vector<Ogre::Renderable *,Ogre::STLAllocator<Ogre::Renderable *,Ogre::CategorisedAllocPolicy<0> > > *>
+		[488]	(0x5006bd20 {mParent=0x50063838 {mPasses={ size=2 } mIlluminationPasses={ size=0 } mParent=0x50039b60 {...} ...} ...}, 0x4fecca68 { size=0 })	std::pair<Ogre::Pass * const,std::vector<Ogre::Renderable *,Ogre::STLAllocator<Ogre::Renderable *,Ogre::CategorisedAllocPolicy<0> > > *>
+		[489]	(0x4e8e6b98 {mParent=0x4e881388 {mPasses={ size=2 } mIlluminationPasses={ size=0 } mParent=0x50063d58 {...} ...} ...}, 0x0e0245a8 { size=1 })	std::pair<Ogre::Pass * const,std::vector<Ogre::Renderable *,Ogre::STLAllocator<Ogre::Renderable *,Ogre::CategorisedAllocPolicy<0> > > *>
+		[490]	(0x3d4c4718 {mParent=0x3d4c3ce0 {mPasses={ size=4 } mIlluminationPasses={ size=0 } mParent=0x3d4c3ad8 {...} ...} ...}, 0x4aa29798 { size=0 })	std::pair<Ogre::Pass * const,std::vector<Ogre::Renderable *,Ogre::STLAllocator<Ogre::Renderable *,Ogre::CategorisedAllocPolicy<0> > > *>
// ...
if you remember

Code: Select all

        struct PassGroupLess
        {
            bool _OgreExport operator()(const Pass* a, const Pass* b) const
            {
                // Sort by passHash, which is pass, then texture unit changes
                uint32 hasha = a->getHash();
                uint32 hashb = b->getHash();
                if (hasha == hashb)
                {
                    // Must differentTransparentQueueItemLessiate by pointer incase 2 passes end up with the same hash
                    return a < b;
                }
                else
                {
                    return hasha < hashb;
                }
            }
        };
This means that the element 487 can not be located between 488 and 489 In addition elements 488 and 489 refer to the shared memory element !!! The container turned out to be inconsistent state.
But in release nothing leads to crush =), while for such a container is not called QueuedRenderableCollection::merge() - it use iteration from begin to end
In my opinion this is a good example UB, isn't? =)

Oke, I digging deeper =)
Here is the log that led me to Ogre::Pass::operator=()
(please, Do not shoot the musician, he plays as he can, logs inserted late at night, after a week without sleep)

Code: Select all


[+ insert to Group] in addRenderable, name:Emissive/Lamps, Hash:493633536, point:4027D5E8
[+ insert to Group] in addRenderable, name:Emissive/Lamps, Hash:493633536, point:40283478
[+ insert to Group] in addRenderable, name:Emissive/Lamps, Hash:270450688, point:40269B70
[firePreFindVisibleObjects] - end
[SceneManager::_renderScene] - end


<! --- user code use *pass1 = *pass2 --> pass1.pointer: 0x40283478, pass1.name: Emissive/Lamps, pass1.hash:493633536 [change to =>] pass2.point: 0xafaef98, pass2.name: Emissive/Lamps, pass2.hash: 0
[~ copy pass] in Pass::operator=(), newName:Emissive/Lamps, name:Emissive/Lamps, newHash:0, hash:493633536, this:40283478
[~ Insert to dirty] in Pass::_dirtyHash(), name:Emissive/Lamps, Hash:0, point:40283478
[~ Insert to dirty] in Pass::_dirtyHash(), name:Emissive/Lamps, Hash:0, point:40283478



----------[RenderTarget::_updateViewport] is :rtt/77668200/RT_0_0 ----------
----------[RenderTarget::_updateViewport] is :rtt/77668992/RT_1_0 ----------
[updateAllControllers] - begin
[~ Insert to dirty] in Pass::_dirtyHash(), name:Emissive/Lamps, Hash:497352704, point:4384F920
[~ Insert to dirty] in Pass::_dirtyHash(), name:Emissive/Lamps, Hash:497352704, point:438CD988
[~ Insert to dirty] in Pass::_dirtyHash(), name:Emissive/Lamps, Hash:497352704, point:438C76F0
[~ Insert to dirty] in Pass::_dirtyHash(), name:Emissive/Lamps, Hash:497352704, point:43862D80
[~ Insert to dirty] in Pass::_dirtyHash(), name:Emissive/Lamps, Hash:497352704, point:438C1438
[~ Insert to dirty] in Pass::_dirtyHash(), name:Emissive/Lamps, Hash:270450688, point:40269B70
[~ Insert to dirty] in Pass::_dirtyHash(), name:Emissive/Lamps, Hash:0, point:40283478
[~ Insert to dirty] in Pass::_dirtyHash(), name:Emissive/Lamps, Hash:493633536, point:4027D5E8
[updateAllControllers] - end
[updateDirtyInstanceManagers] - end
[_updateSceneGraph] - end
[prepareShadowTextures] - end
[setClipPlanes] - end
[~ Clear dirty] Pass::processPendingPassUpdates
[~Change hash] in Pass::_recalculateHash(), name:Emissive/Lamps, prevHash:270450688, curHash:270450688, point:40269B70
[~Change hash] in Pass::_recalculateHash(), name:Emissive/Lamps, prevHash:493633536, curHash:493633536, point:4027D5E8
[~Change hash] in Pass::_recalculateHash(), name:Emissive/Lamps, prevHash:0, curHash:270450688, point:40283478
[~Change hash] in Pass::_recalculateHash(), name:Emissive/Lamps, prevHash:497352704, curHash:497352704, point:4384F920
[~Change hash] in Pass::_recalculateHash(), name:Emissive/Lamps, prevHash:497352704, curHash:497352704, point:43862D80
[~Change hash] in Pass::_recalculateHash(), name:Emissive/Lamps, prevHash:497352704, curHash:497352704, point:438C1438
[~Change hash] in Pass::_recalculateHash(), name:Emissive/Lamps, prevHash:497352704, curHash:497352704, point:438C76F0
[~Change hash] in Pass::_recalculateHash(), name:Emissive/Lamps, prevHash:497352704, curHash:497352704, point:438CD988
RenderQueue::clear
[prepareRenderQueue] - end
RenderQueue::clear
[firePreFindVisibleObjects] - end
[SceneManager::_renderScene] - end
----------[RenderTarget::_updateViewport] is :rtt/880055592/c0/rt0/game RT-main ----------
[updateAllControllers] - begin
[updateAllControllers] - end
[updateDirtyInstanceManagers] - end
[_updateSceneGraph] - end
[prepareShadowTextures] - end
[setClipPlanes] - end
RenderQueue::clear
[prepareRenderQueue] - end
[firePreFindVisibleObjects] - end
[SceneManager::_renderScene] - end
----------[RenderTarget::_updateViewport] is :rtt/880056120/c1/rt0/game RT-main ----------
[updateAllControllers] - begin
[updateAllControllers] - end
[updateDirtyInstanceManagers] - end
[_updateSceneGraph] - end
[prepareShadowTextures] - end
[setClipPlanes] - end
RenderQueue::clear
[prepareRenderQueue] - end
[firePreFindVisibleObjects] - end
[SceneManager::_renderScene] - end
----------[RenderTarget::_updateViewport] is :game RT-main ----------
[updateAllControllers] - begin
[updateAllControllers] - end
[updateDirtyInstanceManagers] - end
[_updateSceneGraph] - end
[prepareShadowTextures] - end
[setClipPlanes] - end
RenderQueue::clear
[prepareRenderQueue] - end
RenderQueue::clear
[+ insert to Group] in addRenderable, name:Emissive/Lamps, Hash:493633536, point:4027D5E8
[=========== in addRenderable, name:Emissive/Lamps, Hash:270450688, point:40283478 ===========]  <===!!!!!!! YES ASSERT HERE!!!!!
[firePreFindVisibleObjects] - end
[SceneManager::_renderScene] - end
Most interesting is the object for pointer 40283478
First - it insert to mGrouped in Ogre::Root::update()

Second - it change through operator=() in user call code (listner frameStarted), but it still inside mGrouped (and nothing before that did not ensure removal from mGrouped). In this operator was changed mHash and mGrouped that contains it became inconsistent.
We can not guarantee the further removal of this item from inconsistent std::map (If you are looking at the dirty list)

Code: Select all

<! --- user code use *pass1 = *pass2 --> pass1.pointer: 0x40283478, pass1.name: Emissive/Lamps, pass1.hash:493633536 [change to =>] pass2.point: 0xafaef98, pass2.name: Emissive/Lamps, pass2.hash: 0
[~ copy pass] in Pass::operator=(), newName:Emissive/Lamps, name:Emissive/Lamps, newHash:0, hash:493633536, this:40283478
All other - net "luck" =) . Dirty list recalc hash for this object and if it remains original value perhaps it could be without assertion's. Or if further calls made mGrouped.clear(), or maybe anything else - thats luck, that is UB =)

Re: Need help to fix UB which leads to crash

Posted: Mon Oct 13, 2014 5:23 pm
by crousser
spacegaier wrote:Can you please create a ticket on JIRA for it and include if possible the minimal coding example required to reproduce the issue?
My English is terrible =) and I'm not sure that I can briefly and clearly describe the problem there.

As for "minimal coding example required to reproduce the issue" ..Our project can not be called simple =) and I hope you understand - bug requires a some number of conditions, which are difficult to reproduce at "the minimal coding example" (but I'll try to find time for this)

If you create ticket for me, or explain the functionality of my mistake in this matter - I will be very grateful.
Thank you!

Re: Need help to fix UB which leads to crash

Posted: Tue Oct 14, 2014 2:47 am
by dark_sylinc
I have a strong feeling this is bug OGRE-89.
A long standing one.

It is getting fixed in the HLMS branch of 2.0; basically because the whole thing was rewritten and improved. All of the offending code doesn't even exist anymore.

But I'm afraid I can't offer a simple solution for 1.x versions.

Took a better look. It's not the same bug, however it is either caused because msDirtyHashList is static (who did that? really!?) or because the whole implementation should be thrown into trash.

Re: Need help to fix UB which leads to crash

Posted: Tue Oct 14, 2014 7:51 am
by crousser
dark_sylinc wrote:I have a strong feeling this is bug OGRE-89.
Yes! I use Ogre 1.9 version
But - No!!!! I use single scene manager BecauseI knew about this bug a year. The first thing I did when I got a stack like this

Code: Select all

OgreMain!Ogre::QueuedRenderableCollection::merge+0x1e46
OgreMain!Ogre::QueuedRenderableCollection::merge+0x18f7
OgreMain!Ogre::QueuedRenderableCollection::merge+0x1589
OgreMain!Ogre::RenderPriorityGroup::sort+0x1b7
OgreMain!Ogre::SceneManager::renderBasicQueueGroupObjects+0x74
OgreMain!Ogre::SceneManager::_renderQueueGroupObjects+0x101
OgreMain!Ogre::SceneManager::renderVisibleObjectsDefaultSequence+0xdd
OgreMain!Ogre::SceneManager::_renderScene+0x6b3
OgreMain!Ogre::Camera::_renderScene+0xda
OgreMain!Ogre::RenderTarget::_updateViewport+0x25
OgreMain!Ogre::RenderTarget::_updateAutoUpdatedViewports+0x38
OgreMain!Ogre::RenderTarget::updateImpl+0x17
0007c49e OgreMain!Ogre::RenderSystem::_updateAllRenderTargets+0x3d
OgreMain!Ogre::Root::_updateAllRenderTargets+0x1b
OgreMain!Ogre::Root::renderOneFrame+0x55
- I check and make sure that the number of scene-managers does not exceed one.

Hmm .. maybe I was not able to explain all clear =( . But a user-accessible assignment operator for the type Ogre::Pass - is one way to crash your application, even when using single scene-mamanger. Regardless, static or non-static msDirtyHashList. The reason is for this bug - immediate changes hash in object without using before this, search and removal of the containers to which it is used as a key-value