Archived discussion

Memory allocation error in TKOpenGl

Other usage issuesSat, 09/24/2011 - 14:004 replies

QuestionAuthorSat, 09/24/2011 - 14:00

Hello,

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.

Ciao

Max

Discussion

4 archived replies

Posts are displayed in their archived order.

01AuthorSat, 09/24/2011 - 19:14

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.

Ciao

Max

02AuthorSat, 09/24/2011 - 19:18

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

Max

03Commenter-1Mon, 09/26/2011 - 14:04

Dear Author,
I would like to inform you that the posted problem is checked and accepted.
The corresponding issue with ID = 22734 has been registered.
Later you can know if the issue is resolved by checking references to the specified ID in OCCT Release Notes. The analysis of the issue will take some time depending on our technical capability and availability of resources.
Regards

04AuthorMon, 09/26/2011 - 14:26

Hi Supervisor,

I posted a patch in another topic that solves the problem in a non-invasive way...
many small changes inside TKOpenGl, but they don't change at all the code mechanics,
just the allocators used, so it could be safely applied.

Ciao

Max