MySQL Bugs: #38598: Falcon overrides C++ new and delete operators

Bug #38598	Falcon overrides C++ new and delete operators
Submitted:	6 Aug 2008 11:17	Modified:	19 Nov 2009 21:11
Reporter:	Vladislav Vaintroub	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Falcon storage engine	Severity:	S2 (Serious)
Version:	6.0	OS:	Solaris
Assigned to:	Olav Sandstå	CPU Architecture:	Any
Tags:	F_PLATFORM

Description:
Falcon overrides C++ global new and delete operators. This has the effect that 
every allocation with new in the same process (not only for Falcon classes) will go through Falcon allocator, which by the way has restricted memory capacities.

So far we had luck and no other storage engine nor server is using global C++ 
new. But in embedded application, people most certainly will do it. 

How to repeat:
read storage/falcon/MemoryManager.h

Suggested fix:
Be good citizen, do not override global new/delete.

- One could have a base class  in Falcon that overrides new/delete and derive other Falcon classes from it.

Or
- don't overwrite new/delete at all. Use standard allocator, the only interesting resource are records and record versions, one can use ALLOCATE_RECORD and DELETE_RECORD macro for them

This is not a bug.  Overriding intrinsic operators is a legitimate
C++ feature.  Only modules compiled with the redefinition get the
special version of new.  It's not contagious, even in an embedded
server.  Our code should not call new during shutdown and other
code can not call our redefined intrinsic.

Our code should not call new during shutdown , this does not happen.
Other code should not call our new during shutdown - this happens unfortunately.
And here is where it crashed, in C++ runtime that clearly does not belong to falcon. I think it was not our purpose that std::basic_filebuf is using our allocator, but this happens.

  [16] Sync::lock(this = 0x8047164, type = Exclusive), line 58 in "Sync.cpp"
  [17] MemMgr::alloc(this = 0x8d296c0, length = 48), line 328 in "MemMgr.cpp"
  [18] MemMgr::allocateDebug(this = 0x8d296c0, size = 24, fileName =
0x8b3ae34 "MemoryManager.h", line = 48), line 539 in "MemMgr.cpp"
  [19] MemMgrAllocateDebug(s = 24U, file = 0x8b3ae34 "MemoryManager.h", line = 48), line 124 in "MemMgr.cpp"
  [20] operator new(s = 24U), line 48 in "MemoryManager.h"
  [21] std::basic_filebuf<wchar_t,std::char_traits<wchar_t>
 >::close(0xfede02d0), at 0xfecec0c6
  [22] std::basic_filebuf<wchar_t,std::char_traits<wchar_t>
 >::~basic_filebuf(0xfede02d0), at 0xfece8917
  [23] std::__Wide_Init::~__Wide_Init(0x8047284), at 0xfed63af2
  ---- hidden frames, use 'where -h' to see them all ----
  [25] __Cimpl::cplus_fini(0xfec4d000, 0xfebb1d7d, 0x80474c8, 0x804734c, 0xfec4d000, 0xfec52200), at 0xfec663eb
  [26] __cplus_fini_at_exit(0xfeffb818, 0x8374dfc, 0x0, 0x0, 0x0, 0x0), at 0xfed7c9b2
  [27] exit(0x0, 0x0), at 0xfeba4a32

Further study of what is done im memory manager revealed following 

Falcon uses some technique that aims to define a new/delete per
translation unit (or source file , for simplicity ;)

To accomplish it, new and delete are defined in header file as inline, with variations (MSVC - static inline , GCC - attribute always_inline). I was not able to find documentation for  this technique  anywhere and compiler-specific tricks seem to suggest a non-standard/non-documented feature.

Also, does not seem to work well with Solaris, at least not if executable is debug compiled.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/53090

2808 Vladislav Vaintroub	2008-09-02
      Bug#38598 - Falcon overrides C++ new and delete operators
      
      Problem : even with  the best attempt to make Falcon-special
      "global" new and delete inline and used only in Falcon-related
      context ,there are compilers that would ignore this attempt
      and generate a global function (SunStudio). This makes Falcon
      new and delete used  by C++ runtime and other storage engines,
      which is bad . Especially bad for allocations/deallocations of
      static objects, where Falcon allocator classes possibly do not
      exist anymore.
      
      Fix: Instead of defining global new and delete, introduce template
      class Allocator with own new and delete operators. This template is
      currently used to define RecordAllocator: base class for everything
      related to the record cache (Record, Value and objects dynamically
      allocated in Value, i.e AsciiBlob, BinaryBlob and BigInt).
      
      
            There is also GeneralAllocator thought for all other classes but
            currently used only as an example in Falcon SQL parser (NNode and
            derivates)

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/53091

2808 Vladislav Vaintroub	2008-09-03
      Bug#38598 - Falcon overrides C++ new and delete operators
      
      Problem : even with  the best attempt to make Falcon-special
      "global" new and delete inline and used only in Falcon-related
      context ,there are compilers that would ignore this attempt
      and generate a global function (SunStudio). This makes Falcon
      new and delete used  by C++ runtime and other storage engines,
      which is bad . Especially bad for allocations/deallocations of
      static objects, where Falcon allocator classes possibly do not
      exist anymore.
      
      Fix: Instead of defining global new and delete, introduce template
      class Allocator with own new and delete operators. This template is
      currently used to define RecordAllocator: base class for everything
      related to the record cache (Record, Value and objects dynamically
      allocated in Value, i.e AsciiBlob, BinaryBlob and BigInt).
      
      
      There is also GeneralAllocator thought for all other classes but
      currently used only as an example in Falcon SQL parser (NNode and
      derivates)

There has been much discussion on this topic in email, IRC and during the Falcon meetings.  We will discuss it more in person at the dev meeting in Riga.