quesa-develop

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

[Sean M. <se...@ro...>]

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!

Thanks!

-- 
____________________________________________________________
Sean McBride, B. Eng                 se...@ro...
Rogue Research                        www.rogue-research.com 
Mac Software Developer              Montréal, Québec, Canada

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?

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

[Sean M. <se...@ro...>]

On 4/14/09 11:20 PM, James W. Walker said:

>The code you're looking at is not in e3geom_trimesh_copydata, but in  
>e3geom_trimesh_copyattributes.

Ack, yes, sorry about that.

>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.

That was my impression too.

>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.

Agreed.

>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).

I don't think this kind of subtraction is ever correct/reliable,
really.  The structure padding is not mandated by the C language, and so
the compiler is free to choose it.  Or am I missing something...?

>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?

The Mac OS X ABI spells out the rules on OS X; other platforms have
other rules of course.  For example, for ppc64:

<http://developer.apple.com/documentation/DeveloperTools/Conceptual/
LowLevelABI/110-64-bit_PowerPC_Function_Calling_Conventions/
64bitPowerPC.html#//apple_ref/doc/uid/TP40002471>

I've just noticed Quesa.h has "#pragma options align=power".  Perhaps
the code really depends on that type of alignment?

-- 
____________________________________________________________
Sean McBride, B. Eng                 se...@ro...
Rogue Research                        www.rogue-research.com 
Mac Software Developer              Montréal, Québec, Canada

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

[Roger H. <rog...@mi...>]

We should probably add an extra field to E3ClassInfo. I suggest it be  
called deltaInstanceSize.

We should have a new parameter in E3ClassTree::RegisterClass. When  
called from E3ClassTree::RegisterExternalClass, this will be passed  
straight through. When called for a built in class which merely adds a  
single object to its base class, it would pass in the sizeof () this  
object. When called for a class which adds more than one object to its  
base class it would have to be the sum of the sizeof ()s of those  
objects, however I don't think this will matter as such a class would  
probably not call GetInstanceSize.

GetInstanceSize would need to be changed to return deltaInstanceSize  
instead of the difference of the instanceSize class and its parent.

OpaqueTQ3Object::FindLeafInstanceData would need to be changed to add  
( instanceSize - deltaInstanceSize ) to 'this' rather than the parent  
class's instanceSize, which would then allow for the pad bytes moving  
the data fields forward in the record.

The same change would need to be made to  
OpaqueTQ3Object::DeleteInstanceData and  
OpaqueTQ3Object::DuplicateInstanceData.

Roger.




On 15 Apr 2009, at 15:56, Sean McBride wrote:

> On 4/14/09 11:20 PM, James W. Walker said:
>
>> The code you're looking at is not in e3geom_trimesh_copydata, but in
>> e3geom_trimesh_copyattributes.
>
> Ack, yes, sorry about that.
>
>> 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.
>
> That was my impression too.
>
>> 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.
>
> Agreed.
>
>> 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).
>
> I don't think this kind of subtraction is ever correct/reliable,
> really.  The structure padding is not mandated by the C language,  
> and so
> the compiler is free to choose it.  Or am I missing something...?
>
>> 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?
>
> The Mac OS X ABI spells out the rules on OS X; other platforms have
> other rules of course.  For example, for ppc64:
>
> <http://developer.apple.com/documentation/DeveloperTools/Conceptual/
> LowLevelABI/110-64-bit_PowerPC_Function_Calling_Conventions/
> 64bitPowerPC.html#//apple_ref/doc/uid/TP40002471>
>
> I've just noticed Quesa.h has "#pragma options align=power".  Perhaps
> the code really depends on that type of alignment?
>
> -- 
> ____________________________________________________________
> Sean McBride, B. Eng                 se...@ro...
> Rogue Research                        www.rogue-research.com
> Mac Software Developer              Montréal, Québec, Canada
>
>
>
> ------------------------------------------------------------------------------
> This SF.net email is sponsored by:
> High Quality Requirements in a Collaborative Environment.
> Download a free trial of Rational Requirements Composer Now!
> http://p.sf.net/sfu/www-ibm-com
> _______________________________________________
> Quesa-develop mailing list
> Que...@li...
> https://lists.sourceforge.net/lists/listinfo/quesa-develop

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

[Sean M. <se...@ro...>]

On 4/15/09 10:29 PM, Roger Holmes said:

>We should probably add an extra field to E3ClassInfo. I suggest it be  
>called deltaInstanceSize.
>
>We should have a new parameter in E3ClassTree::RegisterClass. When  
>called from E3ClassTree::RegisterExternalClass, this will be passed  
>straight through. When called for a built in class which merely adds a  
>single object to its base class, it would pass in the sizeof () this  
>object. When called for a class which adds more than one object to its  
>base class it would have to be the sum of the sizeof ()s of those  
>objects, however I don't think this will matter as such a class would  
>probably not call GetInstanceSize.
>
>GetInstanceSize would need to be changed to return deltaInstanceSize  
>instead of the difference of the instanceSize class and its parent.
>
>OpaqueTQ3Object::FindLeafInstanceData would need to be changed to add  
>( instanceSize - deltaInstanceSize ) to 'this' rather than the parent  
>class's instanceSize, which would then allow for the pad bytes moving  
>the data fields forward in the record.
>
>The same change would need to be made to  
>OpaqueTQ3Object::DeleteInstanceData and  
>OpaqueTQ3Object::DuplicateInstanceData.

This sounds reasonable.  I don't know Quesa well enough to attempt
this... Roger or James, do one of you have the time/inclination?  I can
certainly do a code review and testing.

Cheers,

-- 
____________________________________________________________
Sean McBride, B. Eng                 se...@ro...
Rogue Research                        www.rogue-research.com 
Mac Software Developer              Montréal, Québec, Canada

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

[James W. <ja...@fr...>]

Sean McBride wrote:


> This sounds reasonable.  I don't know Quesa well enough to attempt
> this... Roger or James, do one of you have the time/inclination?  I can
> certainly do a code review and testing.

I don't have time right now, maybe over the weekend.
-- 
   James W. Walker, Innoventive Software LLC
   <http://www.frameforge3d.com/>

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

[Roger H. <rog...@mi...>]

On 16 Apr 2009, at 22:23, Sean McBride wrote:

> On 4/15/09 10:29 PM, Roger Holmes said:
>
>> We should probably add an extra field to E3ClassInfo. I suggest it be
>> called deltaInstanceSize.
>>
>> We should have a new parameter in E3ClassTree::RegisterClass. When
>> called from E3ClassTree::RegisterExternalClass, this will be passed
>> straight through. When called for a built in class which merely  
>> adds a
>> single object to its base class, it would pass in the sizeof () this
>> object. When called for a class which adds more than one object to  
>> its
>> base class it would have to be the sum of the sizeof ()s of those
>> objects, however I don't think this will matter as such a class would
>> probably not call GetInstanceSize.
>>
>> GetInstanceSize would need to be changed to return deltaInstanceSize
>> instead of the difference of the instanceSize class and its parent.
>>
>> OpaqueTQ3Object::FindLeafInstanceData would need to be changed to add
>> ( instanceSize - deltaInstanceSize ) to 'this' rather than the parent
>> class's instanceSize, which would then allow for the pad bytes moving
>> the data fields forward in the record.
>>
>> The same change would need to be made to
>> OpaqueTQ3Object::DeleteInstanceData and
>> OpaqueTQ3Object::DuplicateInstanceData.
>
> This sounds reasonable.  I don't know Quesa well enough to attempt
> this... Roger or James, do one of you have the time/inclination?  I  
> can
> certainly do a code review and testing.

I have forgotten how to use CVS (and never was very confident with it)  
and don't really want to re-learn it, sorry. I am interested in doing  
a 64 bit version of our application and and happy to advise. I guess  
its a lot of work to convert the project to SVN. I could modify my  
local files (some of which have now diverged quite a bit because I  
have none of James' recent changes, but I think this area should be  
safe) and give the changes but thats almost what I've done already. I  
can't really test on 64 bit until we do the rest of the application,  
and we are about a week away from release of a new version, so a bad  
time.

Roger.

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

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

On Apr 17, 2009, at 4:16 AM, Roger Holmes wrote:

> I have forgotten how to use CVS (and never was very confident with it)
> and don't really want to re-learn it, sorry. I am interested in doing
> a 64 bit version of our application and and happy to advise. I guess
> its a lot of work to convert the project to SVN.

If we were going to switch version control systems, I'd prefer to go  
to Mercurial, which I'm now using at work and which is also available  
at SourceForge.  I wrote a Mac GUI for Mercurial.

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

[Lane R. <la...@if...>]

on Fri, Apr 17, 2009 Roger Holmes may have said:

>I have forgotten how to use CVS (and never was very confident with it)  
>and don't really want to re-learn it, sorry. I am interested in doing  
>a 64 bit version of our application and and happy to advise. I guess  
>its a lot of work to convert the project to SVN.

Actually any admin on the SF project can setup the svn quite easily, and
then it is a simple matter to import the latest checkout from CVS.

I would also like to see Quesa in svn, I've stopped checking out updates
due to this.

Lane Roathe
President    Ideas From the Deep  <http://www.ideasfromthedeep.com>
___________________________________________________________________
If we aren't supposed to eat animals, why are they made with meat?

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

[Sean M. <se...@ro...>]

On 4/17/09 12:16 PM, Roger Holmes said:

>> This sounds reasonable.  I don't know Quesa well enough to attempt
>> this... Roger or James, do one of you have the time/inclination?  I  
>> can
>> certainly do a code review and testing.
>
>I have forgotten how to use CVS (and never was very confident with it)  
>and don't really want to re-learn it, sorry. I am interested in doing  
>a 64 bit version of our application and and happy to advise. I guess  
>its a lot of work to convert the project to SVN. I could modify my  
>local files (some of which have now diverged quite a bit because I  
>have none of James' recent changes, but I think this area should be  
>safe) and give the changes but thats almost what I've done already. I  
>can't really test on 64 bit until we do the rest of the application,  
>and we are about a week away from release of a new version, so a bad  
>time.

Well, if after your impending release, you want to give it a go, I can
help with the CVS troubles.  I can zip up the complete Quesa source tree
and put it on our server, you can then download it over http, make your
changes, and send the changes back to me.  I can then review, test, and
generate a patch file for James.  

BTW, who else has commit privileges, besides James?

As for CVS, I'm no fan either.  Switching to svn would be great imho.

-- 
____________________________________________________________
Sean McBride, B. Eng                 se...@ro...
Rogue Research                        www.rogue-research.com 
Mac Software Developer              Montréal, Québec, Canada

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

[James W. <ja...@fr...>]

Sean McBride wrote:


> BTW, who else has commit privileges, besides James?

Joe Strout

Jose Cruanyes

Roger Holmes

Kevin Matthews
-- 
   James W. Walker, Innoventive Software LLC
   <http://www.frameforge3d.com/>

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

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

On Apr 17, 2009, at 4:16 AM, Roger Holmes wrote:

> I have forgotten how to use CVS (and never was very confident with it)
> and don't really want to re-learn it, sorry.

That's pretty much how I feel about Subversion, though it sounds like  
I'm in a minority of one.

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

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

I'm working on the update.  Roger's plan seems sound, except for one  
thing...


On Apr 15, 2009, at 2:29 PM, Roger Holmes wrote:

> When called for a class which adds more than one object to its
> base class it would have to be the sum of the sizeof ()s of those
> objects, however I don't think this will matter as such a class would
> probably not call GetInstanceSize.


I don' t trust the idea of adding up sizeof()s, so in cases where  
there is more than one instance member, I will wrap them up in a  
structure.

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

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

I've done the instance data revision, and posted the changed files here:

<ftp://ftp.jwwalker.com/misc/instance_data_fix.zip>

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

[Sean M. <se...@ro...>]

On 4/19/09 4:46 PM, James W. Walker said:

>I've done the instance data revision, and posted the changed files here:
>
><ftp://ftp.jwwalker.com/misc/instance_data_fix.zip>

Thanks James!

I've reviewed it and it looks ok to me.  It's working great, both in our
new 64 bit app (which uses Quesa only to open 3dmf) and in our old 32
bit app (which uses Quesa a lot).

Will you be checking it into CVS?

Thanks,

-- 
____________________________________________________________
Sean McBride, B. Eng                 se...@ro...
Rogue Research                        www.rogue-research.com 
Mac Software Developer              Montréal, Québec, Canada

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

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

On Apr 21, 2009, at 8:45 AM, Sean McBride wrote:

> On 4/19/09 4:46 PM, James W. Walker said:
>
>> I've done the instance data revision, and posted the changed files  
>> here:
>>
>> <ftp://ftp.jwwalker.com/misc/instance_data_fix.zip>
>
> Thanks James!
>
> I've reviewed it and it looks ok to me.  It's working great, both in  
> our
> new 64 bit app (which uses Quesa only to open 3dmf) and in our old 32
> bit app (which uses Quesa a lot).
>
> Will you be checking it into CVS?


Thanks for reviewing it.  It has been checked in.

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

[Sean M. <se...@ro...>]

On 4/21/09 9:23 AM, James W. Walker said:

>Thanks for reviewing it.  It has been checked in.

Great, thanks.  Did you also commit that small unrelated patch I emailed
you offlist the other day?

-- 
____________________________________________________________
Sean McBride, B. Eng                 se...@ro...
Rogue Research                        www.rogue-research.com 
Mac Software Developer              Montréal, Québec, Canada