OgreInstancedGeometry Memory Corruption.

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
User avatar
0xC0DEFACE
OGRE Expert User
OGRE Expert User
Posts: 84
Joined: Thu May 21, 2009 4:55 am
x 7

OgreInstancedGeometry Memory Corruption.

Post by 0xC0DEFACE »

In response to this thread i have found a memory corruption in OgreInstancedGeometry.cpp

http://www.ogre3d.org/forums/viewtopic.php?f=2&t=60365

See the following switch statement that is in void InstancedGeometry::GeometryBucket::build() :

Code: Select all

         switch (elem.getSemantic())
         {
            case VES_POSITION:
               tmp.x = *pSrcReal++;
               tmp.y = *pSrcReal++;
               tmp.z = *pSrcReal++;
               *pDstReal++ = tmp.x;
               *pDstReal++ = tmp.y;
               *pDstReal++ = tmp.z;
               if(tmp.x<Xmin)
                  Xmin = tmp.x;
               if(tmp.y<Ymin)
                  Ymin = tmp.y;
               if(tmp.z<Zmin)
                  Zmin = tmp.z;
               if(tmp.x>Xmax)
                  Xmax = tmp.x;
               if(tmp.y>Ymax)
                  Ymax = tmp.y;
               if(tmp.z>Zmax)
                  Zmax = tmp.z;
            default:
            // just raw copy
            memcpy(pDstReal, pSrcReal,
               VertexElement::getTypeSize(elem.getType()));
            break;
         };
As you can see if the vertex semantic is VES_POSITION pDstReal will be incremented 3 times and then the switch will fall through and incorrectly use the changed pDstReal to do a second copy.

Removing the following 3 lines fixes the issue:

Code: Select all

       *pDstReal++ = tmp.x;
       *pDstReal++ = tmp.y;
       *pDstReal++ = tmp.z;
User avatar
AshMcConnell
Silver Sponsor
Silver Sponsor
Posts: 605
Joined: Fri Dec 14, 2007 11:44 am
Location: Northern Ireland
x 16
Contact:

Re: OgreInstancedGeometry Memory Corruption.

Post by AshMcConnell »

I'll admit I don't fully know what's going on in that code, but when I removed the 3 lines my verts were all over the place, am I missing something?

Thanks!
Ash
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: OgreInstancedGeometry Memory Corruption.

Post by dark_sylinc »

Yes, the way it handles the copy is a bit obscure. But removing those 3 lines is not the solution.

I'm shooting blind, but if there is truly corruption, it's very likely the code should then look like:

Code: Select all

*pDstRea = tmp.x;
*(pDstReal+1) = tmp.y;
*(pDstReal+2) = tmp.z;
User avatar
AshMcConnell
Silver Sponsor
Silver Sponsor
Posts: 605
Joined: Fri Dec 14, 2007 11:44 am
Location: Northern Ireland
x 16
Contact:

Re: OgreInstancedGeometry Memory Corruption.

Post by AshMcConnell »

dark_sylinc wrote:Yes, the way it handles the copy is a bit obscure. But removing those 3 lines is not the solution.

I'm shooting blind, but if there is truly corruption, it's very likely the code should then look like:

Code: Select all

*pDstRea = tmp.x;
*(pDstReal+1) = tmp.y;
*(pDstReal+2) = tmp.z;
Nope, unfortunately similar results compared to removing the lines.
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: OgreInstancedGeometry Memory Corruption.

Post by dark_sylinc »

Ok, I took the time to read the original thread this time.

The loop seems perfectly fine.
Assuming this is the actual cause of the memory corruption, the best possible explanation is the allocated buffer is not large enough.

The best way to confirm is to put a variable and increase it whith each iteration, and then compare to the size of the buffer.

Edit: Meaning, the ones who have time and kindness to debug it are welcome to :lol:
If someone doesn't understand what the loop is doing, it's very simple. It tries to do a raw copy of the src vertex data. However there are 2 exceptions:
1. The extra texture coordinate which is used to individualize each instance inside the shader
2. The position is copied exactly, but it is done manually so it can calculate a growing bounding box.
User avatar
0xC0DEFACE
OGRE Expert User
OGRE Expert User
Posts: 84
Joined: Thu May 21, 2009 4:55 am
x 7

Re: OgreInstancedGeometry Memory Corruption.

Post by 0xC0DEFACE »

dark_sylinc wrote:Yes, the way it handles the copy is a bit obscure. But removing those 3 lines is not the solution.

I'm shooting blind, but if there is truly corruption, it's very likely the code should then look like:

Code: Select all

*pDstRea = tmp.x;
*(pDstReal+1) = tmp.y;
*(pDstReal+2) = tmp.z;
While that will also fix the problem, it will still fall through and do the copy twice. I would punt that AshMcConnell is victim of an unrelated issue.

That being said it is a clear buffer overrun, and removing the 3 lines does indeed fix it. The buffers are the correct size.

Lets simplify this.

Assume a buffer with one vert and that vert only has a position therefore the two buffers (the source and the destination ) are 12 bytes.

The switch will be hit once.

The position gets set once like this

Code: Select all

//pDstReal is pointing to byte 0 here
*pDstReal++ = tmp.x; 
// pDstReal is pointing to byte 4 here
*pDstReal++ = tmp.y;
// pDstReal is pointing to byte 8 here
*pDstReal++ = tmp.z; 
// pDstReal is pointing to byte 12 here which is 1 byte past where this iteration of the loop should ever access.
// the switch then falls through to the default case and does this. (the 12 for simplicity)
memcpy(pDstReal, pSrcReal, 12 );
Clearly it is reading 12 bytes past the end of one buffer and writing 12 bytes past the end of the other.

The reason removing the 3 lines works is because then then memcpy does the exact same thing as they would have.
It can also be fixed by adding a break before the default case. The only reason I didn't opt for that solution is that i much prefer removing 3 lines of code to fix a bug rather than adding 1. lol.

This is also why your solution works, because it doesn't change the pointer so the memcpy doesn't overrun, but it still ends up doing a copy twice.
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: OgreInstancedGeometry Memory Corruption.

Post by dark_sylinc »

What you said made no sense to me..... until I saw the bug. Missing break:

Code: Select all

/* Continues from above */
    if(tmp.z<Zmin)
		Zmin = tmp.z;
	if(tmp.x>Xmax)
		Xmax = tmp.x;
	if(tmp.y>Ymax)
		Ymax = tmp.y;
	if(tmp.z>Zmax)
		Zmax = tmp.z;
	break;         //-----> Here!!!
default:
// just raw copy
memcpy(pDstReal, pSrcReal,
	VertexElement::getTypeSize(elem.getType()));
break;
Another alternative solution is to avoid the break, remove those 3 lines like you said, and also change this:

Code: Select all

tmp.x = *pSrcReal;
tmp.y = *(pSrcReal+1;
tmp.z = *(pSrcReal+2);
Removing the 3 lines caused rectangles to get spiky because pSrcReal shouldn't be incremented if we'll later do a memset

Cheers
Dark Sylinc
User avatar
0xC0DEFACE
OGRE Expert User
OGRE Expert User
Posts: 84
Joined: Thu May 21, 2009 4:55 am
x 7

Re: OgreInstancedGeometry Memory Corruption.

Post by 0xC0DEFACE »

Oh wow I totally didn't notice pSrcReal being incremented, so yep the break is the ideal fix.

I did what you said to stop pSrcReal incrementing, But used nicer syntax ;)

Code: Select all

	tmp.x = pSrcReal[0];
	tmp.y = pSrcReal[1];
	tmp.z = pSrcReal[2];
cool!
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: OgreInstancedGeometry Memory Corruption.

Post by dark_sylinc »

Since AshMcConnell has been having trouble with this fix, I'll wait for his confirmation that it now works as expected before comiting any change.

PS: I wonder if this is the reason some old instancing code of mine worked on NVIDIA cards but didn't in ATI cards...
User avatar
AshMcConnell
Silver Sponsor
Silver Sponsor
Posts: 605
Joined: Fri Dec 14, 2007 11:44 am
Location: Northern Ireland
x 16
Contact:

Re: OgreInstancedGeometry Memory Corruption.

Post by AshMcConnell »

Yep, this works for me. I wonder if this is the (seemingly) memory corruption problem that was messing with my Driver Aids (traction control).

Quite a nasty bug, nice find!

PS. Just to be sure, the fix is just a case of adding the break as the first case statement is doing a copy anyway?
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: OgreInstancedGeometry Memory Corruption.

Post by dark_sylinc »

I made sure the instancing demo worked before the fix, breaking the code (which didn't, so this ensures me this code actually was getting called in the demo), and after the fix.

Pushed the fix into stable 1.7
User avatar
0xC0DEFACE
OGRE Expert User
OGRE Expert User
Posts: 84
Joined: Thu May 21, 2009 4:55 am
x 7

Re: OgreInstancedGeometry Memory Corruption.

Post by 0xC0DEFACE »

Great work man.
Post Reply