User-allocated ACE_Data_Block mis-deleted at release time about ace_tao HOT 12 CLOSED

Branch issue-748 created and has the Message_Block_Test.cpp test case addition.

from ace_tao.

Riverace customer case 2124.

from ace_tao.

Thinking... changing the data_block_allocator argument from default 0 to default ACE_Allocator::instance() has the possibility of losing an error from ACE_Allocator::instance() at run time. Can anyone think of a use case where this is a real risk?

from ace_tao.

I probably don't understand the problem yet, but the default ACE_Allocator is ACE_New_Allocator. It only allocates arrays of bytes. So indeed it's correct that it uses new char[] and delete[]. After memory is allocated, objects may be created in those arrays of bytes using standard C++ placement-new (and destroyed using its inverse operation, explicit destruct). If you're going to integrate with the default ACE_Allocator, your own objects should behave in similar ways. The other option would be to have a custom ACE_Allocator.

from ace_tao.

Thank you for your quick response, Adam! Indeed, mandating that user-derived data blocks must be allocated with an ACE_Allocator-derived class would avoid the problem. But is that a reasonable thing to require? Is it documented anywhere that it is required now? I didn't see anything but did not look exhaustively.

The problem is a user can derive something from ACE_Data_Block, allocate one using regular, default operator new, and pass that to a new ACE_Message_Block (which does have the capability to use regular operator delete, BTW). When the ACE_Data_Block is eventually freed, it is done using ACE_Allocator, not the matching operator delete matching the allocation. AddressSanitizer (corrrectly, IMO) picks up on this as a problem.

from ace_tao.

Changing the default looks the wrong direction. @mcorino and me see two alternatives:

• Use a custom ACE_Allocator as also @mitza-oci suggest
• Provide an operator new overload on the user derived class

from ace_tao.

Why is changing the behavior the wrong direction?

from ace_tao.

I would like to look at the code in question but branch issue-748 has no commits that aren't already on master.

from ace_tao.

Oops, my bad - sorry, Adam. I just pushed the added test case for this - tests/Message_Block_Test.cpp

from ace_tao.

Redefining 0 to trigger a delete instead of using the default ACE allocator instance could probably break other users and would be a different behavior as in all other places in ACE. Personally I would go for custom allocator solution.

from ace_tao.

Besides what we've discussed already, the following simple change to the test makes the allocation and deallocation consistent. Also the DONT_DELETE flag can be used to have the application take care of data block deletion instead of ACE_Message_Block doing it.

  ACE_NEW_MALLOC_RETURN (user_data_block,
                         static_cast<User_Data *> (
                           ACE_Allocator::instance ()->malloc (sizeof (User_Data))),
                         User_Data (),
                         -1);

from ace_tao.

Thank you for the code change example, Adam! I added this to the Message_Block_Test.cpp test case and proposed this to the customer.

We can consider this issue "user error" at this point.

from ace_tao.