MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0022734Community[OCCT] OCCT:Visualizationpublic2011-09-26 13:482012-03-30 10:16
Reporterszy 
Assigned Todbv 
PrioritynormalSeveritymajor 
StatusclosedResolutionfixed 
PlatformAOSLOS VersionL
Product Version[OCCT] 6.5.1 
Target Version[OCCT] 6.5.3Fixed in Version[OCCT] 6.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 Filespatch file icon TKOpenGl.patch (59,323 bytes) 2011-09-26 18:18

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

-  Notes
(0018648)
san (developer)
2011-11-29 09:45
edited on: 2012-02-21 16:11

Dear APL,

Please consider this issue as urgent one.

(0018997)
san (developer)
2011-12-26 11:23

DC,

Finally, I suggest freezing this issue and waiting for implementation of 0022819.
(0019701)
san (developer)
2012-02-21 16:47

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().
(0020024)
dbv (developer)
2012-03-16 17:48

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.
(0020107)
san (developer)
2012-03-22 22:08

CR22734 reviewed with some intermediate remarks, the remarks have been corrected, branch is ready for testing.
(0020124)
apn (administrator)
2012-03-23 17:24

Dear san,
     I have merged branch CR22734 with master to solve problems with compilation.
Please review this patch.
(0020131)
apn (administrator)
2012-03-26 10:36

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 : ----------------------------------------------------
(0020165)
apn (administrator)
2012-03-27 15:50

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
(0020177)
san (developer)
2012-03-27 18:10

OpenGl_TextureBox.cxx changes from master restored: "const" modifiers lost by the previous rebase rolled back.
(0020178)
san (developer)
2012-03-27 18:10

CR22734_Rebased corrected, please test
(0020181)
aan (tester)
2012-03-28 12:02

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
Timestamp: 2012-03-29 15:29:26
Author: dbv
Details ] Diff ]
0022734: Memory allocation error in OpenGl
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 View Revisions
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 View Revisions
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


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker