Re: Buffer overrun in 64 bit build: problem found, solution lacking

[James W. W. <os...@jw...>]

On Apr 14, 2009, at 3:50 PM, Sean McBride wrote:

> Hi all,
>
> So we've built Quesa as 64 bit on Mac OS X 10.5.6 and found a nasty  
> bug
> when loading 3dmf files.  I've found the cause, but I am not sure of  
> the
> proper solution.
>
> Superficially, the problem is that a buffer of x bytes is allocated  
> but
> then later it attempts to copy that buffer, but with a size greater  
> than x.
>
> The allocation is performed in e3fformat_3dmf_attributearray_read(),
> specifically:
>
> 		case kQ3AttributeTypeNormal:			// TQ3Vector3D
> 			theAttribute->data = Q3Memory_Allocate
>     (sizeof(TQ3Vector3D) * numElems);
>
> Based on my very limited understanding, this seems correct.  Also,  
> note
> that sizeof(TQ3Vector3D) is 12 bytes both in 32 and 64 bit.
>
> This buffer is later copied (I confirmed it's the same buffer) in
> e3geom_trimesh_copydata():
>
> 		TQ3Uns32 attrSize = theClass->GetInstanceSize () ;
> 		TQ3Uns32 bytes    = numElements * attrSize ;
> 		if ( bytes != 0 )
> 			qd3dStatus = e3geom_trimesh_clone(
>        srcAttributeTypes[i].data,
> 								&(*destAttributeTypes)[i].data,
> 								bytes);
>
> The error is this computation of 'attrSize'.  In 32 bit,
> GetInstanceSize() gives 12, but in 64 bit it gives 16.  That seems
> correct.  It seems to me merely luck that GetInstanceSize() ==
> sizeof(TQ3Vector3D) in 32 bit.  I don't understand why  
> GetInstanceSize()
> is used here, or for that matter, what that function is all about.
>
> If I change:
>
> 		TQ3Uns32 attrSize = theClass->GetInstanceSize () ;
>
> to:
>
> 		TQ3Uns32 attrSize = sizeof(TQ3Vector3D) ;
>
> My problem is 'solved'.  But I suspect this is not a general fix.
>
> I have a small test project that repros 100%, if someone (James? :))
> would care to take a look!




The code you're looking at is not in e3geom_trimesh_copydata, but in  
e3geom_trimesh_copyattributes.  I guess it's trying to get the size of  
the data for any given attribute type, in a way that would be  
compatible with custom attributes.  It would definitely be a bad idea  
to replace theClass->GetInstanceSize() by sizeof(TQ3Vector3D), because  
it won't be right for other attributes than normal vectors, such as UV  
coordinates.  If you only care about the built-in attribute types, you  
could use an array lookup or switch statement to get the right size,  
but it would be nice to figure out what's going wrong.  The class in  
question is E3NormalAttribute, a subclass of E3Attribute with one  
additional member, a TQ3Vector3D.  It looks like GetInstanceSize  
basically takes sizeof(E3NormalAttribute) - sizeof(E3Attribute).   
Apparently, in the 64-bit case, 4 pad bytes were added between the  
base class and the instance data of the subclass. What exactly are the  
rules about when structures are padded in the 64-bit world?