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.

OgreInstancedGeometry Memory Corruption.

Postby 0xC0DEFACE » Wed Sep 22, 2010 5:02 am

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

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
0xC0DEFACE
OGRE Expert User
OGRE Expert User
 
Posts: 82
Kudos: 7
Joined: 21 May 2009

Re: OgreInstancedGeometry Memory Corruption.

Postby AshMcConnell » Wed Sep 22, 2010 11:44 am

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
AshMcConnell
Hobgoblin
 
Posts: 572
Kudos: 11
Joined: 14 Dec 2007
Location: Northern Ireland

Re: OgreInstancedGeometry Memory Corruption.

Postby dark_sylinc » Wed Sep 22, 2010 6:21 pm

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
dark_sylinc
OGRE Team Member
OGRE Team Member
 
Posts: 3220
Kudos: 490
Joined: 21 Jul 2007
Location: Buenos Aires, Argentina

Re: OgreInstancedGeometry Memory Corruption.

Postby AshMcConnell » Wed Sep 22, 2010 8:30 pm

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
AshMcConnell
Hobgoblin
 
Posts: 572
Kudos: 11
Joined: 14 Dec 2007
Location: Northern Ireland

Re: OgreInstancedGeometry Memory Corruption.

Postby dark_sylinc » Wed Sep 22, 2010 10:40 pm

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
dark_sylinc
OGRE Team Member
OGRE Team Member
 
Posts: 3220
Kudos: 490
Joined: 21 Jul 2007
Location: Buenos Aires, Argentina

Re: OgreInstancedGeometry Memory Corruption.

Postby 0xC0DEFACE » Thu Sep 23, 2010 6:19 am

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
0xC0DEFACE
OGRE Expert User
OGRE Expert User
 
Posts: 82
Kudos: 7
Joined: 21 May 2009

Re: OgreInstancedGeometry Memory Corruption.

Postby dark_sylinc » Thu Sep 23, 2010 3:37 pm

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
dark_sylinc
OGRE Team Member
OGRE Team Member
 
Posts: 3220
Kudos: 490
Joined: 21 Jul 2007
Location: Buenos Aires, Argentina

Re: OgreInstancedGeometry Memory Corruption.

Postby 0xC0DEFACE » Fri Sep 24, 2010 1:05 am

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
0xC0DEFACE
OGRE Expert User
OGRE Expert User
 
Posts: 82
Kudos: 7
Joined: 21 May 2009

Re: OgreInstancedGeometry Memory Corruption.

Postby dark_sylinc » Fri Sep 24, 2010 2:42 am

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
dark_sylinc
OGRE Team Member
OGRE Team Member
 
Posts: 3220
Kudos: 490
Joined: 21 Jul 2007
Location: Buenos Aires, Argentina

Re: OgreInstancedGeometry Memory Corruption.

Postby AshMcConnell » Fri Sep 24, 2010 7:47 am

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
AshMcConnell
Hobgoblin
 
Posts: 572
Kudos: 11
Joined: 14 Dec 2007
Location: Northern Ireland

Re: OgreInstancedGeometry Memory Corruption.

Postby dark_sylinc » Fri Sep 24, 2010 11:57 pm

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
dark_sylinc
OGRE Team Member
OGRE Team Member
 
Posts: 3220
Kudos: 490
Joined: 21 Jul 2007
Location: Buenos Aires, Argentina

Re: OgreInstancedGeometry Memory Corruption.

Postby 0xC0DEFACE » Tue Sep 28, 2010 1:39 am

Great work man.
User avatar
0xC0DEFACE
OGRE Expert User
OGRE Expert User
 
Posts: 82
Kudos: 7
Joined: 21 May 2009


Return to Developer talk

Who is online

Users browsing this forum: Google [Bot] and 3 guests