Need help to fix UB which leads to crash

Discussion area about developing or extending OGRE, adding plugins for it or building applications on it. No newbie questions please, use the Help forum for that.
Post Reply
crousser
Gnoblar
Posts: 12
Joined: Tue Dec 18, 2007 2:05 pm

Need help to fix UB which leads to crash

Post 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
User avatar
spacegaier
OGRE Team Member
OGRE Team Member
Posts: 4304
Joined: Mon Feb 04, 2008 2:02 pm
Location: Germany
x 135
Contact:

Re: Need help to fix UB which leads to crash

Post 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?
Ogre Admin [Admin, Dev, PR, Finance, Wiki, etc.] | BasicOgreFramework | AdvancedOgreFramework
Don't know what to do in your spare time? Help the Ogre wiki grow! Or squash a bug...
User avatar
mmixLinus
Silver Sponsor
Silver Sponsor
Posts: 199
Joined: Thu Apr 21, 2011 3:08 pm
Location: Lund, Sweden
x 12
Contact:

Re: Need help to fix UB which leads to crash

Post by mmixLinus »

Powered by Ogre3D:
MMiX.Me 3D - 3D Music Player
Galaxy Navigator 3D - 2 million stars (ESA's Gaia satellite)
YouTube|Facebook
crousser
Gnoblar
Posts: 12
Joined: Tue Dec 18, 2007 2:05 pm

Re: Need help to fix UB which leads to crash

Post 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 =)
Last edited by crousser on Mon Oct 13, 2014 5:33 pm, edited 2 times in total.
crousser
Gnoblar
Posts: 12
Joined: Tue Dec 18, 2007 2:05 pm

Re: Need help to fix UB which leads to crash

Post 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!
User avatar
dark_sylinc
OGRE Team Member
OGRE Team Member
Posts: 5296
Joined: Sat Jul 21, 2007 4:55 pm
Location: Buenos Aires, Argentina
x 1278
Contact:

Re: Need help to fix UB which leads to crash

Post 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.
crousser
Gnoblar
Posts: 12
Joined: Tue Dec 18, 2007 2:05 pm

Re: Need help to fix UB which leads to crash

Post 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
Post Reply