fixed memory leaks and memory measurement leaks by removing -fsized-deallocation

 * Array allocations were not properly accounted since the compiler
   would call the replacement new() which assumes that it would
   always be called along with the replacement sized-deletion.
   This is not true for array new[] allocations resulting in
   a constant increase of memory_usage and unrecoverable situations.
   This problem however could be fixed in principle by avoiding
   memory counting for arrays or falling back to malloc_usable_size.
 * The bigger problem was that some STLs (new_allocator) are broken, calling the
   non-sized delete for regular new() calls which could in principle
   be matched by sized-delete.
   This is also the reason why I had to provide a non-sized
   delete replacement, which in reality intoduced memory leaks.
 * Since adding checks for the broken compiler versions or a configure-time
   check that tries to detect these broken systems seems tedious,
   I simply removed that optimization.
 * This means we always have to rely on malloc_usable_size() now
   for non-SciTECO-object memory measurement.
 * Perhaps in the future, there should be an option for allowing
   portable measurement at the cost of memory usage, by prefixing
   each memory chunk with the chunk size.
   Maintainers could then decide to optimize their build for "speed"
   at the cost of memory overhead.
 * Another solution to this non-ending odyssey might be to introduce
   our own allocator, replacing malloc(), and allowing our own
   precise measurements.

2 files changed, 28 insertions, 63 deletions

diff --git a/configure.ac b/configure.ac
index c341183..032ab10 100644
--- a/configure.ac
+++ b/configure.ac
@@ -181,29 +181,6 @@ esac
 AC_CHECK_HEADERS([malloc.h malloc_np.h])
 AC_CHECK_FUNCS([malloc_trim malloc_usable_size])
 
-# Check whether compiler supports global sized deallocations.
-# If yes, this will improve the memory limiting fallback
-# implementation.
-# Once we can depend on C++14, this check is no longer necessary.
-AC_CACHE_CHECK([if C++ compiler supports -fsized-deallocation],
-               [sciteco_cv_sized_deallocation_result], [
-	AC_LANG_PUSH(C++)
-	OLD_CXXFLAGS="$CXXFLAGS"
-	CXXFLAGS="$CXXFLAGS -fsized-deallocation"
-	AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
-		[[#include <new>]],
-		[[(::operator delete)(0, 256)]]
-	)], sciteco_cv_sized_deallocation_result=yes,
-	    sciteco_cv_sized_deallocation_result=no)
-	CXXFLAGS="$OLD_CXXFLAGS"
-	AC_LANG_POP(C++)
-])
-if [[ x$sciteco_cv_sized_deallocation_result = xyes ]]; then
-	AC_DEFINE(HAVE_SIZED_DEALLOCATION, 1,
-	          [C++ compiler supports -fsized-deallocation])
-	CXXFLAGS="$CXXFLAGS -fsized-deallocation"
-fi
-
 #
 # Config options
 #
diff --git a/src/memory.cpp b/src/memory.cpp
index b592d9e..fd7adf7 100644
--- a/src/memory.cpp
+++ b/src/memory.cpp
@@ -132,6 +132,18 @@ MemoryLimit memlimit;
  *   Even the malloc_usable_size() workaround for old or non-GNU
  *   compilers is still faster than mallctl() on FreeBSD.
  *   This might need to change in the future.
+ * - Beginning with C++14 (or earlier with -fsized-deallocation),
+ *   it is possible to globally replace sized allocation/deallocation
+ *   functions, which could be used to avoid the malloc_usable_size()
+ *   workaround. Unfortunately, this may not be used for arrays,
+ *   since the compiler may have to call non-sized variants if the
+ *   original allocation size is unknown - and there is no way to detect
+ *   that when the new[] call is made.
+ *   What's worse is that at least G++ STL is broken seriously and
+ *   some versions will call the non-sized delete() even when sized-deallocation
+ *   is available. Again, this cannot be detected at new() time.
+ *   Therefore, I had to remove the sized-deallocation based
+ *   optimization.
  */
 
 #ifdef G_OS_WIN32
@@ -179,12 +191,19 @@ MemoryLimit::get_usage(void)
  * Unfortunately, in the worst case, this will only measure the heap used
  * by C++ objects in SciTECO's sources; not even Scintilla, nor all
  * g_malloc() calls.
- * Usually, we will be able to use global sized deallocators or
+ * Usually, we will be able to use global non-sized deallocators with
  * libc-specific support to get more accurate results, though.
  */
 
 #define MEMORY_USAGE_FALLBACK
 
+/**
+ * Current memory usage in bytes.
+ *
+ * @bug This only works in single-threaded applications.
+ *      Should SciTECO or Scintilla ever use multiple threads,
+ *      it will be necessary to use atomic operations.
+ */
 static gsize memory_usage = 0;
 
 gsize
@@ -234,7 +253,9 @@ MemoryLimit::check(void)
  * The object-specific sized deallocators allow memory
  * counting portably, even in strict C++11 mode.
  * Once we depend on C++14, they and the entire `Object`
- * class hack can be avoided.
+ * class hack may be avoided.
+ * But see above - due to broken STLs, this may not actually
+ * be safe!
  */
 
 void *
@@ -290,50 +311,19 @@ Object::operator delete(void *ptr, size_t size) noexcept
 
 } /* namespace SciTECO */
 
-#ifdef HAVE_SIZED_DEALLOCATION
-/*
- * Global sized deallocators must be defined in the
- * root namespace.
- * Since we only depend on C++11, we cannot just always
- * define them (they are a C++14 feature).
- * They will effectively measure all C++ allocations,
- * including the ones in Scintilla which can make a huge
- * difference.
- * Due to the poor platform support for memory measurement,
- * it's still worth to support them optionally.
- */
-
-void *
-operator new(size_t size)
-{
-	return SciTECO::Object::operator new(size);
-}
-
-void
-operator delete(void *ptr, size_t size) noexcept
-{
-	SciTECO::Object::operator delete(ptr, size);
-}
-
-/*
- * FIXME: I'm a bit puzzled of why this is necessary.
- * Apparently, G++ sometimes calls the non-sized,
- * FOLLOWED BY the sized deallocator even when
- * building Scintilla with -fsized-deallocation.
- * It is still uncertain whether the compiler may
- * call the non-sized WITHOUT the sized variant.
- */
-void operator delete(void *ptr) noexcept {}
-
-#else
 /*
  * In strict C++11, we can still use global non-sized
  * deallocators.
+ *
  * On their own, they bring little benefit, but with
  * some libc-specific functionality, they can be used
  * to improve the fallback memory measurements to include
  * all allocations (including Scintilla).
  * This comes with a moderate runtime penalty.
+ *
+ * Unfortunately, even in C++14, defining replacement
+ * sized deallocators may be very dangerous, so this
+ * seems to be as best as we can get (see above).
  */
 
 void *
@@ -358,5 +348,3 @@ operator delete(void *ptr) noexcept
 #endif
 	g_free(ptr);
 }
-
-#endif /* !HAVE_SIZED_DEALLOCATION */