View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0022734CommunityOCCT:Visualizationpublic2011-09-26 13:482012-03-30 10:16
Reporterszy Assigned Todbv 
PrioritynormalSeveritymajor 
Status closedResolutionfixed 
PlatformAOSL 
Product Version6.5.1 
Target Version6.5.3Fixed in Version6.5.3 
Summary0022734: Memory allocation error in OpenGl
DescriptionPost 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()."

==============
TKOpenGl.patch (proposed by the community) is attached for information.
TagsNo tags attached.
Test case numbertest case is not required

Attached Files

  • TKOpenGl.patch (59,323 bytes)

Relationships

related to 0022234 closedbugmaster Open CASCADE Compilation warnings in OpenGl unit 
related to 0022819 closedbugmaster Open CASCADE Redesign of OpenGl driver 
related to 0022815 closedbugmaster Community Missing delete operator for placement new 
related to 0022627 closedbugmaster Open CASCADE Change OCCT memory management defaults 

Activities

szy

2011-09-26 18:18

manager  

TKOpenGl.patch (59,323 bytes)

san

2011-11-29 09:45

developer   ~0018648

Last edited: 2012-02-21 16:11

 Dear APL,

Please consider this issue as urgent one.

san

2011-12-26 11:23

developer   ~0018997

DC,

Finally, I suggest freezing this issue and waiting for implementation of 0022819.

san

2012-02-21 16:47

developer   ~0019701

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

dbv

2012-03-16 17:48

developer   ~0020024

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.

san

2012-03-22 22:08

developer   ~0020107

CR22734 reviewed with some intermediate remarks, the remarks have been corrected, branch is ready for testing.

apn

2012-03-23 17:24

administrator   ~0020124

Dear san,
     I have merged branch CR22734 with master to solve problems with compilation.
Please review this patch.

apn

2012-03-26 10:36

administrator   ~0020131

Dear BugMaster,
      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 : ----------------------------------------------------

apn

2012-03-27 15:50

administrator   ~0020165

Dear BugMaster,
       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

san

2012-03-27 18:10

developer   ~0020177

OpenGl_TextureBox.cxx changes from master restored: "const" modifiers lost by the previous rebase rolled back.

san

2012-03-27 18:10

developer   ~0020178

CR22734_Rebased corrected, please test

aan

2012-03-28 12:02

tester   ~0020181

Dear BugMaster,

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 Changesets

occt: master 9d35f668

2012-03-29 15:29:26

dbv

Details Diff		 0022734: Memory allocation error in OpenGl Affected Issues
0022734
mod - src/OpenGl/FILES Diff File
mod - src/OpenGl/OpenGl_GraphicDriver_9.cxx Diff File
mod - src/OpenGl/OpenGl_GraphicDriver_Layer.cxx Diff File
mod - src/OpenGl/OpenGl_ImageBox.cxx Diff File
rm - src/OpenGl/OpenGl_Memory.hxx Diff File
mod - src/OpenGl/OpenGl_Polygon.cxx Diff File
mod - src/OpenGl/OpenGl_Polygon.hxx Diff File
mod - src/OpenGl/OpenGl_PrimitiveArray.cxx Diff File
mod - src/OpenGl/OpenGl_Text.cxx Diff File
mod - src/OpenGl/OpenGl_TextureBox.cxx Diff File

Issue History

Date Modified Username Field Change
2011-09-26 13:48 szy New Issue
2011-09-26 13:48 szy Assigned To => san
2011-09-26 18:18 szy Description Updated
2011-09-26 18:18 szy File Added: TKOpenGl.patch
2011-09-27 11:47 kgv Relationship added related to 0022627
2011-09-27 12:02 szy Assigned To san => kgv
2011-09-27 12:02 szy Status new => assigned
2011-10-07 16:35 san Relationship added related to 0022234
2011-10-07 16:38 san Assigned To kgv => dln
2011-11-07 11:34 san Assigned To dln => san
2011-11-29 09:43 san Assigned To san => apl
2011-11-29 09:45 san Note Added: 0018648
2011-12-26 11:22 san Relationship added related to 0022819
2011-12-26 11:23 san Note Added: 0018997
2011-12-26 11:24 san Assigned To apl => san
2012-02-21 11:53 kgv Relationship added related to 0022815
2012-02-21 16:11 san Note Edited: 0018648
2012-02-21 16:11 san Assigned To san => dbv
2012-02-21 16:47 san Note Added: 0019701
2012-03-16 17:48 dbv Note Added: 0020024
2012-03-16 17:48 dbv Assigned To dbv => san
2012-03-16 17:48 dbv Status assigned => resolved
2012-03-22 22:08 san Note Added: 0020107
2012-03-22 22:08 san Assigned To san => bugmaster
2012-03-22 22:08 san Status resolved => reviewed
2012-03-23 12:09 mkv Assigned To bugmaster => apn
2012-03-23 17:24 apn Note Added: 0020124
2012-03-23 17:24 apn Status reviewed => assigned
2012-03-23 17:24 apn Assigned To apn => san
2012-03-23 17:24 apn Status assigned => resolved
2012-03-23 18:26 san Assigned To san => bugmaster
2012-03-23 18:26 san Status resolved => reviewed
2012-03-26 09:56 apn Assigned To bugmaster => apn
2012-03-26 10:36 apn Note Added: 0020131
2012-03-26 10:37 apn Assigned To apn => dbv
2012-03-26 10:37 apn Status reviewed => assigned
2012-03-26 17:39 dbv Status assigned => resolved
2012-03-26 17:41 dbv Assigned To dbv => apn
2012-03-26 17:41 dbv Status resolved => reviewed
2012-03-27 11:44 mkv Assigned To apn => aan
2012-03-27 12:55 mkv Assigned To aan => apn
2012-03-27 15:50 apn Note Added: 0020165
2012-03-27 15:51 apn Assigned To apn => dbv
2012-03-27 15:51 apn Status reviewed => assigned
2012-03-27 18:10 san Note Added: 0020177
2012-03-27 18:10 san Status assigned => resolved
2012-03-27 18:10 san Note Added: 0020178
2012-03-27 18:10 san Assigned To dbv => bugmaster
2012-03-27 18:10 san Status resolved => reviewed
2012-03-27 18:19 apn Assigned To bugmaster => apn
2012-03-28 12:02 aan Note Added: 0020181
2012-03-28 12:03 aan Test case number => test case is not required
2012-03-28 12:03 aan Status reviewed => tested
2012-03-29 09:34 apn Assigned To apn => bugmaster
2012-03-30 10:16 dbv Changeset attached => occt master 9d35f668
2012-03-30 10:16 dbv Assigned To bugmaster => dbv
2012-03-30 10:16 dbv Status tested => verified
2012-03-30 10:16 dbv Resolution open => fixed