Archived issue #0022734
Memory allocation error in OpenGl
Description
Post from the Forum - http://www.opencascade.org/org/forum/thread_21881/.
1.
"... using opencascade together with a toolkit that does memory allocation optimizations, I found a bug in memory allocations inside OpenGL.
In file InterfaceGraphic_telem.hxx there is a macro that defines allocators for
structs TEL_xxxx, declared as following :
#define IMPLEMENT_MEMORY_OPERATORS \
void* operator new (size_t size) {\
void* p = malloc( size );\
memset(p, 0, size);\
return p;\
}\
void operator delete(void* p) {\
free( p );\
}
so, using malloc() and free() for allocation.
The problem arises because of this declaration :
typedef union TSM_ELEM_DATA_UNION
{
void *pdata;
Tint ldata;
} TSM_ELEM_DATA, *tsm_elem_data;
where pdata points to TEL_xxx structures.
So, when you allocate the TEL_xxx structures, the correct
allocator defined for class is used; but when you delete 'pdata'
(done in *many* parts of code), being this a void * pointer
(which is wrong anyways to be deleted by operator delete() ),
the wrong deallocator is used, ::operator delete().
In other parts deletion is done by free(data), which is also wrong,
because the whole stuff is mixing allocation done by TEL_xxx, globals
new() and delete() and free().
That give no problem if global operator delete() points to
free(), but brings memory corruptions if global operator delete()
is redefined.
I can post a patch, if interested, that solves both the warnings and allocation bugs.
I guess the most correct thing would be to make pdata a pointer to, for example,
TS_ELEM_DATA structure and add allocators to this one too; then deleting pdata would
be correct. We shall also replace the free(pdata) with delete(pdata) everywhere.
"
2.
"Looking deeper inside it, we've got another problem.... some memory in opengl
is allocated by realloc(), which has no direct equivalent in new/delete part.
So, we've got 3 choices, to fix it correctly :
1) replace ALL malloc/calloc/free with their operator new and so counterpart; replace realloc with new+memcpy
and set operators as above; that'll require many, many small changes in most TKOpenGl files.
2) the way around, replace ALL occurrences of new/delete with malloc/free, forget about IMPLEMENT_MEMORY_OPERATORS, they won't be used anymore, and hope that no memory allocated by opengl will be
freed outside it.
3)same as 2, but making an opengl allocator, end use that one. Not many advantages than 2, but like this you
can change later the allocation schema, which is not bad.
The option of leaving it alone is, IMHO, the worse; linking opencascade with *any* library that does memory
management overriding globals new and delete *will* bring problems, crashes and memory leaks.
Most "modern" solution would be the 1, imho, with the expense of copy-on-realloc (but usually it happens anyways within realloc) and the advantage that you don't have problem mixing allocations from inside/outside opengl."
3.
"Here an example of how it's wrong there; in OpenGl_Polygon.cxx :
if( s->tmesh_sequence == 0 )
{
s->tmesh_sequence = new void*[s->ts_alloc];
}
else
{
#if defined(__SUNPRO_CC) && (__SUNPRO_CC <= 0x530)
s->tmesh_sequence = (void**)realloc( s->tmesh_sequence, s->ts_alloc*sizeof(void*));
#else
s->tmesh_sequence = cmn_resizemem<void*>( s->tmesh_sequence, s->ts_alloc);
#endif
}
Inside same function, we're allocating with new() and reallocating with realloc, which is plenty
wrong and works *only* if operator new() === malloc() and operator delete() === free()."
==============
Attachment 1 (PATCH) (proposed by the community) is attached for information.
1.
"... using opencascade together with a toolkit that does memory allocation optimizations, I found a bug in memory allocations inside OpenGL.
In file InterfaceGraphic_telem.hxx there is a macro that defines allocators for
structs TEL_xxxx, declared as following :
#define IMPLEMENT_MEMORY_OPERATORS \
void* operator new (size_t size) {\
void* p = malloc( size );\
memset(p, 0, size);\
return p;\
}\
void operator delete(void* p) {\
free( p );\
}
so, using malloc() and free() for allocation.
The problem arises because of this declaration :
typedef union TSM_ELEM_DATA_UNION
{
void *pdata;
Tint ldata;
} TSM_ELEM_DATA, *tsm_elem_data;
where pdata points to TEL_xxx structures.
So, when you allocate the TEL_xxx structures, the correct
allocator defined for class is used; but when you delete 'pdata'
(done in *many* parts of code), being this a void * pointer
(which is wrong anyways to be deleted by operator delete() ),
the wrong deallocator is used, ::operator delete().
In other parts deletion is done by free(data), which is also wrong,
because the whole stuff is mixing allocation done by TEL_xxx, globals
new() and delete() and free().
That give no problem if global operator delete() points to
free(), but brings memory corruptions if global operator delete()
is redefined.
I can post a patch, if interested, that solves both the warnings and allocation bugs.
I guess the most correct thing would be to make pdata a pointer to, for example,
TS_ELEM_DATA structure and add allocators to this one too; then deleting pdata would
be correct. We shall also replace the free(pdata) with delete(pdata) everywhere.
"
2.
"Looking deeper inside it, we've got another problem.... some memory in opengl
is allocated by realloc(), which has no direct equivalent in new/delete part.
So, we've got 3 choices, to fix it correctly :
1) replace ALL malloc/calloc/free with their operator new and so counterpart; replace realloc with new+memcpy
and set operators as above; that'll require many, many small changes in most TKOpenGl files.
2) the way around, replace ALL occurrences of new/delete with malloc/free, forget about IMPLEMENT_MEMORY_OPERATORS, they won't be used anymore, and hope that no memory allocated by opengl will be
freed outside it.
3)same as 2, but making an opengl allocator, end use that one. Not many advantages than 2, but like this you
can change later the allocation schema, which is not bad.
The option of leaving it alone is, IMHO, the worse; linking opencascade with *any* library that does memory
management overriding globals new and delete *will* bring problems, crashes and memory leaks.
Most "modern" solution would be the 1, imho, with the expense of copy-on-realloc (but usually it happens anyways within realloc) and the advantage that you don't have problem mixing allocations from inside/outside opengl."
3.
"Here an example of how it's wrong there; in OpenGl_Polygon.cxx :
if( s->tmesh_sequence == 0 )
{
s->tmesh_sequence = new void*[s->ts_alloc];
}
else
{
#if defined(__SUNPRO_CC) && (__SUNPRO_CC <= 0x530)
s->tmesh_sequence = (void**)realloc( s->tmesh_sequence, s->ts_alloc*sizeof(void*));
#else
s->tmesh_sequence = cmn_resizemem<void*>( s->tmesh_sequence, s->ts_alloc);
#endif
}
Inside same function, we're allocating with new() and reallocating with realloc, which is plenty
wrong and works *only* if operator new() === malloc() and operator delete() === free()."
==============
Attachment 1 (PATCH) (proposed by the community) is attached for information.
Public activity
11 archived notes
Participants are labeled by their role within this record.
Dear APL,
Please consider this issue as urgent one.
Please consider this issue as urgent one.
Dear DBV,
After finishing #0022815, please, consider memory usage problems in the redesigned graphic driver (OpenGl package).
Proposed correction sequence:
- OpenGl_Context::Init() allocates two structures and stores them in arbVBO and extFBO fields. These pointers should be deleted by ~OpenGl_Context().
- OpenGl_Memory.hxx, OpenGl_Polygon.cxx, OpenGl_TextureBox.cxx: realloc() is used to change size of a memory block allocated with operator new. It is suggested to use cmn_resizemem() instead, and change its implementation to use new and delete internally.
- Check OpenGl sources for unmatched new and delete calls.
- Think if it is possible to use a unified memory allocation/deallocation scheme in OpenGl package: either new/delete or malloc()/free(). Currently, malloc() is called in few OpenGl source files only, and maybe it could be replaced safely with new.
- Finally, think if it is possible to switch macros in InterfaceGraphic_telem.hxx to using Standard:Allocate() and Standard::Free().
After finishing #0022815, please, consider memory usage problems in the redesigned graphic driver (OpenGl package).
Proposed correction sequence:
- OpenGl_Context::Init() allocates two structures and stores them in arbVBO and extFBO fields. These pointers should be deleted by ~OpenGl_Context().
- OpenGl_Memory.hxx, OpenGl_Polygon.cxx, OpenGl_TextureBox.cxx: realloc() is used to change size of a memory block allocated with operator new. It is suggested to use cmn_resizemem() instead, and change its implementation to use new and delete internally.
- Check OpenGl sources for unmatched new and delete calls.
- Think if it is possible to use a unified memory allocation/deallocation scheme in OpenGl package: either new/delete or malloc()/free(). Currently, malloc() is called in few OpenGl source files only, and maybe it could be replaced safely with new.
- Finally, think if it is possible to switch macros in InterfaceGraphic_telem.hxx to using Standard:Allocate() and Standard::Free().
1. Added deletion of pointers arbVBO and extFBO
2. malloc()/free() have been replaced with new()/delete()
3. Structures and pointers, which served as a vector of elements, have been replaced with NCollection_Vector.
4. src/OpenGl/OpenGl_Memory.hxx has been removed, since it's no longer needed.
Git branch CR22734 is ready to be reviewed.
Dear SAN,
Please review.
2. malloc()/free() have been replaced with new()/delete()
3. Structures and pointers, which served as a vector of elements, have been replaced with NCollection_Vector.
4. src/OpenGl/OpenGl_Memory.hxx has been removed, since it's no longer needed.
Git branch CR22734 is ready to be reviewed.
Dear SAN,
Please review.
CR22734 reviewed with some intermediate remarks, the remarks have been corrected, branch is ready for testing.
Dear san,
I have merged branch CR22734 with master to solve problems with compilation.
Please review this patch.
I have merged branch CR22734 with master to solve problems with compilation.
Please review this patch.
Dear Commenter 1,
Workbench KAS:dev:apn-22734-occt was created from git branch CR22734 (and apn-22734-products from svn trunk) and compiled on Linux platform.
There is compilation error:
Error : Failed : OpenGl_Text.cxx
/dn47/KAS/dev/apn-22734-occt/src/OpenGl/OpenGl_Polygon.cxx: In destructor 'virtual OpenGl_Polygon::~OpenGl_Polygon()':
/dn47/KAS/dev/apn-22734-occt/src/OpenGl/OpenGl_Polygon.cxx:567: warning: deleting 'void* const' is undefined
Info : ----------------------- Compilation Report -----------------------
Info : Failed : :KAS:dev:apn-22734-occt:OpenGl:source:OpenGl_Text.cxx
Info : Failed : :KAS:dev:apn-22734-occt:OpenGl:source:OpenGl_TextureBox.cxx
Info : -----------------------------------------------------------------
Info : ------------------ Process report ------------------
Info : Failed OpenGl (obj.comp obj.idep )
Info : ----------------------------------------------------
Workbench KAS:dev:apn-22734-occt was created from git branch CR22734 (and apn-22734-products from svn trunk) and compiled on Linux platform.
There is compilation error:
Error : Failed : OpenGl_Text.cxx
/dn47/KAS/dev/apn-22734-occt/src/OpenGl/OpenGl_Polygon.cxx: In destructor 'virtual OpenGl_Polygon::~OpenGl_Polygon()':
/dn47/KAS/dev/apn-22734-occt/src/OpenGl/OpenGl_Polygon.cxx:567: warning: deleting 'void* const' is undefined
Info : ----------------------- Compilation Report -----------------------
Info : Failed : :KAS:dev:apn-22734-occt:OpenGl:source:OpenGl_Text.cxx
Info : Failed : :KAS:dev:apn-22734-occt:OpenGl:source:OpenGl_TextureBox.cxx
Info : -----------------------------------------------------------------
Info : ------------------ Process report ------------------
Info : Failed OpenGl (obj.comp obj.idep )
Info : ----------------------------------------------------
Dear Commenter 1,
Workbench KAS:dev:apn-22734-occt was created from git branch CR22734_Rebased (and apn-22734-products from svn trunk) and compiled on Linux platform.
I have some problems with compilation (link libTKOpenGl.so):
Check of undefined symbols with LD_LIBRARY_PATH : /misc/dn47/KAS/dev/apn-22734-occt/lin/lib::/dn22/KAS/BAG/wok-m2010/lib/lin:/PRODUCTS/maintenance/Mandriva2010/freeimage/lib:/PRODUCTS/maintenance/Mandriva2010/tbb/lib/ia32/cc4.1.0_libc2.4_kernel2.6.16.21:/dn29/PRODUCTS/maintenance/Mandriva2010/qt-4.6.2/lib:/PRODUCTS/maintenance/Mandriva2010/ftgl-2.1.2/lib:/PRODUCTS/maintenance/Mandriva2010/gl2ps-1.3.5/lib:/PRODUCTS/maintenance/Mandriva2010/tcltk-85/lib:/dn22/KAS/BAG/wok-m2010/lib/lin:/usr/lib:/dn22/KAS/BAG/wok-m2010/lib/lin:/usr/lib:/usr/X11R6/lib:/lib:/PRODUCTS/Linux/Xau/lib:/dn22/KAS/BAG/wok-m2010/lib/lin::
/misc/dn47/KAS/dev/apn-22734-occt/lin/lib/libTKOpenGl.so: undefined reference to `SetModeEye(int, float const*, float const*)'
/misc/dn47/KAS/dev/apn-22734-occt/lin/lib/libTKOpenGl.so: undefined reference to `SetModeObject(int, float const*, float const*)'
Check failed
Workbench KAS:dev:apn-22734-occt was created from git branch CR22734_Rebased (and apn-22734-products from svn trunk) and compiled on Linux platform.
I have some problems with compilation (link libTKOpenGl.so):
Check of undefined symbols with LD_LIBRARY_PATH : /misc/dn47/KAS/dev/apn-22734-occt/lin/lib::/dn22/KAS/BAG/wok-m2010/lib/lin:/PRODUCTS/maintenance/Mandriva2010/freeimage/lib:/PRODUCTS/maintenance/Mandriva2010/tbb/lib/ia32/cc4.1.0_libc2.4_kernel2.6.16.21:/dn29/PRODUCTS/maintenance/Mandriva2010/qt-4.6.2/lib:/PRODUCTS/maintenance/Mandriva2010/ftgl-2.1.2/lib:/PRODUCTS/maintenance/Mandriva2010/gl2ps-1.3.5/lib:/PRODUCTS/maintenance/Mandriva2010/tcltk-85/lib:/dn22/KAS/BAG/wok-m2010/lib/lin:/usr/lib:/dn22/KAS/BAG/wok-m2010/lib/lin:/usr/lib:/usr/X11R6/lib:/lib:/PRODUCTS/Linux/Xau/lib:/dn22/KAS/BAG/wok-m2010/lib/lin::
/misc/dn47/KAS/dev/apn-22734-occt/lin/lib/libTKOpenGl.so: undefined reference to `SetModeEye(int, float const*, float const*)'
/misc/dn47/KAS/dev/apn-22734-occt/lin/lib/libTKOpenGl.so: undefined reference to `SetModeObject(int, float const*, float const*)'
Check failed
OpenGl_TextureBox.cxx changes from master restored: "const" modifiers lost by the previous rebase rolled back.
CR22734_Rebased corrected, please test
Dear Commenter 1,
Workbench KAS:dev:apn-22734-occt was created from git branch CR22734_Rebased
(and apn-22734-products from svn trunk) and compiled on Linux platform.
There are not regressions in apn-22734-products regarding to
KAS:dev:products-20120326-opt
See results in
/QADisk/occttests/results/KAS/dev/apn-22734-products_27032012/lin
See reference results in
/QADisk/occttests/results/KAS/dev/products-20120326-opt_26032012/lin
See test cases in /QADisk/occttests/tests/ED
N.B. In order to launch testing case you can make use the following
instructions
http://doc/doku.php?id=occt:certification
Workbench KAS:dev:apn-22734-occt was created from git branch CR22734_Rebased
(and apn-22734-products from svn trunk) and compiled on Linux platform.
There are not regressions in apn-22734-products regarding to
KAS:dev:products-20120326-opt
See results in
/QADisk/occttests/results/KAS/dev/apn-22734-products_27032012/lin
See reference results in
/QADisk/occttests/results/KAS/dev/products-20120326-opt_26032012/lin
See test cases in /QADisk/occttests/tests/ED
N.B. In order to launch testing case you can make use the following
instructions
http://doc/doku.php?id=occt:certification
Related records