[PATCH 06/25] delete void* is undefined

Quick comment on this one - the change looks quite suspicious, so I would encourage deeper code review. Dynamic memory must be allocated and deallocated using consistent pairs (new/delete, malloc/free, new[]/delete[]).
Mixing new and free is bad practice and can lead to undefined behavior. Even if for simple POD, Plain Old Data, structures it can work fine, for any complex types it may be generally unpredictable.

I have quickly looked at OpenGl_addnames.cxx and see that allocation in AddNamesetAdd() is done view new :

static TStatus AddNamesetAdd( TSM_ELEM_DATA d, Tint n, cmn_key *k )
{
...
data = new TEL_TINT_DATA();

...
((tsm_elem_data)(d.pdata))->pdata = data;
...
}

deallocation (in the patch) via free:
static TStatus AddNamesetDelete( TSM_ELEM_DATA data, Tint n, cmn_key *k )
{
if (data.pdata)
free(data.pdata);
return TSuccess;
}

This is wrong. The better way would be to use
detete reinterpret_cast (data.pdata);

or C-style cast: delete (TEL_TINT_DATA*) (data.pdata);

With that, I would recommend a patch rework. Ideally, it would be good to use the memory checker (like Intel Inspector XE or probably Valgrind) to detect inconsistent behavior.

Hope this helps.
Commenter-1

Hello Roman,

You are right, mixing new/free is bad practice, but well this patch is better than nothing and does its job. The main problem here is the usage of void*, C++ inheritance would be much better.

If someone wants to provide an improved version of this patch, that would be great, but I will not do that myself, it is frustrating to see improvements being ignored by OCC team without knowing why (see http://www.opencascade.org/org/forum/thread_20101/ for instance, they applied a wrong fix in OCCT 6.5.1).

Hi Author,

I see and share your frustration. Be assured, my modifications also get integrated into OCC very slowly causing extra migration efforts (for this reason I am still on 6.5.0). Truth to be told though, I look forward into post-6.5.1 release where most modifications should hopefully be in.

As far as this particular fix is concerned, I believe this (wrong) patch makes it worse than before, as it hides the problem rather than solves it. So I'd rather encourage Hugues (the path's author) to apply inheritance, as you suggest, if it can be applied, or use explicit casts.

HTH.
Commenter-1

> As far as this particular fix is concerned, I believe this (wrong) patch makes it worse than before, as it hides the problem rather than solves it. So I'd rather encourage Hugues (the path's author) to apply inheritance, as you suggest, if it can be applied, or use explicit casts.

The original problem is that deleting a void* pointer has an undefined behavior. This patch fixes it. It could be improved, yes, but in my opinion, this patch is worth applying as is.
Moreover, removing those warnings helps finding real bugs.

Hello, Author,
please, explain your viewpoint more thoroughly. Why do you think our fix in OCCT 6.5.1 is wrong? Please, consider the following explanation below.

The reason why we cannot use XOpenDisplay( NULL ) call in OSD_FontMgr is that we need to enumerate the font files located on the machine where the application is executed - that is, on the "localhost" in terms of code execution.

The NULL argument value implies that the existing X connection is used.

There is no difference when an application is run locally.

However, if an application is run remotely, the existing X connection usually leads us to the wrong machine: a GUI application normally has the X display on the local machine, while the application code is executed on a remote machine.

As a result, we obtain the wrong font files (located on the X server side!), which cannot be accessed later by FreeType and FTGL from the localhost machine.

Please, Author, explain your idea in more detail as well.

Thanks Commenter-2 for this detailed explanation.

Hardcoding DISPLAY to ":0.0" does not make sense to me. For security reasons one should not have access to this display on remote machines, and it may even be possible that the remote machine does not run any X server.

Please note that it will not work locally if one uses an X display different from :0.0; this situation becomes quite common nowadays (on Linux desktops), session managers allow users to create a new session if someone locked down his session on :0.0.

Anyway I realize now that XOpenDisplay(NULL) may not be the best fix; there is no reason to use X stuff if you do not want to use the current X display. A compile-time option or an environment variable look like better alternatives, and as a bonus, these will drop the dependency against X11 in TKernel which had been added in OCCT 6.5.0.

+1

Hello Roman,

Take a look at header inc/InterfaceGraphic_telem.hxx, it contains the macro IMPLEMENT_MEMORY_OPERATORS used in the data structures like TSM_ELEM_DATA so they get equipped with the new() and delete() C++ operators.

Those memory operators are wrappers around malloc() and free(). Am I missing something or are they just C++ syntactic sugar ? In case of sugar then using free() instead of delete is correct then. Allocation should be fixed too nonetheless.

Hi Hugues,

The point in the new operator is zero-filling. But as it was defined, the delete operator had to be defined as well, to ensure consistency.

Callers should not rely on internal implementations of new and delete. So whenever an object has been allocated with new/new[], it be must be deallocated with delete/delete[].

What about the following fix - have a base struct (struct FOO_BAR) that would define these operators, all other structs that currently use the IMPLEMENT_MEMORY_OPERATORS macro would just inherit it (no need to define new/delete again). Then in the struct that currently have void* field, use a typed pointer FOO_BAR*. In this case former caller's code (using delete) will remain unchanged. Will that work ?

HTH.
Commenter-1

Hi Roman,

That seems the way to go. Those warnings could be solved more cleanly (a pity to introduce a base struct just for this, but ...).

I tried to implement your proposition, please see an excerpt here : http://pastebin.com/03J3VQqj

Unfortunately, build fails in OpenGl_textfont.cxx at line 62 :

static TStatus
TextFontAdd( TSM_ELEM_DATA d, Tint n, cmn_key *k )
{
((tsm_elem_data)(d.pdata))->pdata = k[0]->data.pdata; // line 62

return TSuccess;
}

Error : In function ‘TStatus TextAdd(TSM_ELEM_DATA, Tint, CMN_KEY**)’:
src/OpenGl/OpenGl_textfont.cxx:62:50: error: invalid conversion from ‘void*’ to ‘BASE_TSM_ELEM_DATA*’

There is a problem here, pdata is supposed to be actually of type TSM_ELEM_DATA*, but TSM_ELEM_DATA is an union so it can't inherit from BASE_TSM_ELEM_DATA.

Hi,

I've found this post just after doing mine about the same problem.
The quickest solution would be to define pdata as TSM_ELEM_DATA *,
patch some assigments (some 20-30...) inside TKOpenGL
(easy, those are spotted by compiler) ensure that data pointed
by pdata is created with new(SomeClassThatHas_IMPLEMENT_MEMORY_OPERATORS)
and then freed by delete and *NOT* free().
Both 2 last points are difficult to spot into code, indeed.... inside OpenGL
stuffs there are really many mixed allocations, with new/delete and malloc/free, which
is *not* just cosmetic sugar but can lead to crash if global allocators (::operator new/delete)
are redefined by a linked library.

Max

Max