VertexDataManager::prepareVertexData will trigger buffer overflow issue

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Run GLBenchmark on windows with reference device

What is the expected output? What do you see instead?
Memory access error report

What version of the product are you using? On what operating system?
Win7 64
Please provide any additional information below.
Fix code in attached file for libglesv2\VertexDataManager.cpp

Original issue reported on code.google.com by yore.a...@gmail.com on 8 Jul 2011 at 2:47

Attachments:

• angle.fix.txt

[GoogleCodeExporter]

Bug 104 and 139 and 179 are same

Original comment by yore.a...@gmail.com on 8 Jul 2011 at 2:57

[GoogleCodeExporter]

Can you please provide the fix in unified diff (diff -u) against the current 
source? Thanks!

Original comment by dan...@transgaming.com on 8 Jul 2011 at 4:34

[GoogleCodeExporter]

Sure.Wait.

Original comment by yore.a...@gmail.com on 8 Jul 2011 at 5:36

[GoogleCodeExporter]

The "mStreamingBuffer->addRequiredSpaceFor(staticBuffer);" line already grows 
the streaming buffer to include space for the invalidated static buffer. 
Remaining space for attributes of the invalidated buffer is also accumulated to 
the streaming buffer in subsequent iterations.

So I'm not sure which scenario your patch would fix...

Original comment by nicolas....@gmail.com on 8 Jul 2011 at 6:45

[GoogleCodeExporter]

I attached diff file. Thanks!

Original comment by yore.a...@gmail.com on 8 Jul 2011 at 7:19

Attachments:

• patch.diff

[GoogleCodeExporter]

The staticBuffer will be shared among other attributes, Right?
If previous attribute didn't take the path of " if (staticBuffer->size() == 0) 
{...}", how can their required space be recorded?
We test the fix and problem solved. 
Thanks!

Original comment by yore.a...@gmail.com on 8 Jul 2011 at 7:28

[GoogleCodeExporter]

Yes, a static buffer can contain multiple attributes. But staticBuffer->size() 
returns the already allocated space, not the required space accumulator. So it 
doesn't change in this loop, and all the required space for each of the 
attributes gets accumulated in the 
"staticBuffer->addRequiredSpace(spaceRequired(attribs[i], totalCount));" line.

The other path is taken only when a static buffer has already been allocated, 
but doesn't contain all of the requested attributes. An example would be if you 
have a buffer object containing position and color, and you first draw using 
only the position attribute and in another draw call use both attributes. In 
that case the static buffer only contains the position and has to be 
invalidated, and the space for  old static buffer (position) and the new space 
(color) has to be reserved in the streaming buffer.

I'll check whether using 'totalCount' instead of 'count' could cause any 
trouble...

Original comment by nicolas....@gmail.com on 8 Jul 2011 at 9:34

[GoogleCodeExporter]

I think I found the actual bug. mRequiredSpace is reset to 0 in 
StaticVertexBuffer::reserveRequiredSpace(), while that value is actually 
required when the static buffer is invalidated and the required space needs to 
be added to the streaming buffer.

One way to fix it is to not reset it to 0, but I think Yore's patch might be 
better than the original intended approach since it doesn't overestimate the 
required space. Thanks!

Original comment by nicolas....@gmail.com on 8 Jul 2011 at 10:07

• Changed state: Started
• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

I think there's a small bug in your patch. I believe line 18 should use 
'buffer_local' instead of 'buffer'. It does fix the overflow bug but 
potentially reserves too much space.

I've attached a new patch which should correct this and removes 
addRequiredSpaceFor(). Could you test it with GLBenchmark?

Original comment by nicolas....@gmail.com on 8 Jul 2011 at 11:23

Attachments:

• 0001-overflow.patch

[GoogleCodeExporter]

Should be fixed in r702.

Original comment by nicolas....@gmail.com on 8 Jul 2011 at 4:34

• Changed state: Fixed

[GoogleCodeExporter]

OK, I will test your new patch in our Monday.

Original comment by yore.a...@gmail.com on 9 Jul 2011 at 2:18

[GoogleCodeExporter]

Great to have our soluation accepted. 

Original comment by yore.a...@gmail.com on 9 Jul 2011 at 2:20