correctly check sized delete hint when asserts are on

We previously tested wrong assumption that larger than page size size
classes have addresses aligned on page size. New code is making proper
check of size class.

Also added is unit test coverage for this previously failing
condition. And we now also run "assert-ful" unittests for big tcmalloc
too, not only tcmalloc_minimal configuration.

This fixes github issue gperftools#1254

.gitignore

@@ -125,6 +125,8 @@
 /stack_trace_table_test.exe
 /stacktrace_unittest
 /system_alloc_unittest
+/tcm_asserts_unittest
+/tcm_asserts_unittest.exe
 /tcm_min_asserts_unittest
 /tcm_min_asserts_unittest.exe
 /tcmalloc_and_profiler_unittest

Makefile.am

@@ -929,6 +929,21 @@ endif !BUILD_EMERGENCY_MALLOC
 
 ### Making the library
 
+if WITH_HEAP_CHECKER
+# heap-checker-bcad is last, in hopes its global ctor will run first.
+# (Note this is added to libtcmalloc.la, not libtcmalloc_internal.la,
+# but that's ok; the internal/external distinction is only useful for
+# cygwin, and cygwin doesn't use HEAP_CHECKER anyway.)
+HEAP_CHECKER_SOURCES = src/base/thread_lister.c \
+                       src/base/linuxthreads.cc \
+                       src/heap-checker.cc \
+                       src/heap-checker-bcad.cc
+MAYBE_NO_HEAP_CHECK =
+else !WITH_HEAP_CHECKER
+HEAP_CHECKER_SOURCES =
+MAYBE_NO_HEAP_CHECK = -DNO_HEAP_CHECK
+endif !WITH_HEAP_CHECKER
+
 noinst_LTLIBRARIES += libtcmalloc_internal.la
 libtcmalloc_internal_la_SOURCES = $(libtcmalloc_minimal_internal_la_SOURCES) \
                                   $(TCMALLOC_INCLUDES) \
@@ -939,31 +954,34 @@ libtcmalloc_internal_la_SOURCES = $(libtcmalloc_minimal_internal_la_SOURCES) \
                                   $(EMERGENCY_MALLOC_CC) \
                                   src/memory_region_map.cc
 libtcmalloc_internal_la_CXXFLAGS = $(PTHREAD_CFLAGS) -DNDEBUG \
-                                   $(AM_CXXFLAGS) $(EMERGENCY_MALLOC_DEFINE)
+                                   $(AM_CXXFLAGS) $(EMERGENCY_MALLOC_DEFINE) \
+                                   $(MAYBE_NO_HEAP_CHECK)
 libtcmalloc_internal_la_LDFLAGS = $(PTHREAD_CFLAGS)
 libtcmalloc_internal_la_LIBADD = libstacktrace.la $(PTHREAD_LIBS)
 
 lib_LTLIBRARIES += libtcmalloc.la
-libtcmalloc_la_SOURCES = $(TCMALLOC_CC) $(TCMALLOC_INCLUDES)
-libtcmalloc_la_CXXFLAGS = $(PTHREAD_CFLAGS) -DNDEBUG $(AM_CXXFLAGS) $(EMERGENCY_MALLOC_DEFINE)
+libtcmalloc_la_SOURCES = $(TCMALLOC_CC) $(TCMALLOC_INCLUDES) \
+                         $(HEAP_CHECKER_SOURCES)
+libtcmalloc_la_CXXFLAGS = $(PTHREAD_CFLAGS) -DNDEBUG $(AM_CXXFLAGS) \
+                          $(MAYBE_NO_HEAP_CHECK) $(EMERGENCY_MALLOC_DEFINE)
 libtcmalloc_la_LDFLAGS = $(PTHREAD_CFLAGS) -version-info @TCMALLOC_SO_VERSION@
 libtcmalloc_la_LIBADD = libtcmalloc_internal.la libmaybe_threads.la $(PTHREAD_LIBS)
 
-if WITH_HEAP_CHECKER
-# heap-checker-bcad is last, in hopes its global ctor will run first.
-# (Note this is added to libtcmalloc.la, not libtcmalloc_internal.la,
-# but that's ok; the internal/external distinction is only useful for
-# cygwin, and cygwin doesn't use HEAP_CHECKER anyway.)
-HEAP_CHECKER_SOURCES = src/base/thread_lister.c \
-                       src/base/linuxthreads.cc \
-                       src/heap-checker.cc \
-                       src/heap-checker-bcad.cc
-libtcmalloc_la_SOURCES += $(HEAP_CHECKER_SOURCES)
-else !WITH_HEAP_CHECKER
-HEAP_CHECKER_SOURCES =
-libtcmalloc_internal_la_CXXFLAGS += -DNO_HEAP_CHECK
-libtcmalloc_la_CXXFLAGS += -DNO_HEAP_CHECK
-endif !WITH_HEAP_CHECKER
+# same as above with without -DNDEBUG
+noinst_LTLIBRARIES += libtcmalloc_internal_with_asserts.la
+libtcmalloc_internal_with_asserts_la_SOURCES = $(libtcmalloc_internal_la_SOURCES)
+libtcmalloc_internal_with_asserts_la_CXXFLAGS = $(PTHREAD_CFLAGS) \
+                                   $(AM_CXXFLAGS) $(EMERGENCY_MALLOC_DEFINE) \
+                                   $(MAYBE_NO_HEAP_CHECK)
+libtcmalloc_internal_with_asserts_la_LDFLAGS = $(PTHREAD_CFLAGS)
+libtcmalloc_internal_with_asserts_la_LIBADD = libstacktrace.la $(PTHREAD_LIBS)
+
+noinst_LTLIBRARIES += libtcmalloc_with_asserts.la
+libtcmalloc_with_asserts_la_SOURCES = $(libtcmalloc_la_SOURCES)
+libtcmalloc_with_asserts_la_CXXFLAGS =  $(PTHREAD_CFLAGS) $(AM_CXXFLAGS) \
+                          $(MAYBE_NO_HEAP_CHECK) $(EMERGENCY_MALLOC_DEFINE)
+libtcmalloc_with_asserts_la_LDFLAGS = $(PTHREAD_CFLAGS)
+libtcmalloc_with_asserts_la_LIBADD = libtcmalloc_internal_with_asserts.la libmaybe_threads.la $(PTHREAD_LIBS)
 
 LIBTCMALLOC = libtcmalloc.la
 
@@ -997,18 +1015,16 @@ tcmalloc_unittest_LDFLAGS = $(PTHREAD_CFLAGS) $(TCMALLOC_FLAGS)
 # first linkline to make sure our malloc 'wins'.
 tcmalloc_unittest_LDADD = $(LIBTCMALLOC) liblogging.la $(PTHREAD_LIBS)
 
-# # lets make sure we exerice ASSERTs in at least in statically linked
-# # configuration
-# TESTS += tcm_asserts_unittest
-# tcm_asserts_unittest_SOURCES = src/tests/tcmalloc_unittest.cc \
-#                                 src/tests/testutil.h src/tests/testutil.cc \
-#                                 $(libtcmalloc_internal_la_SOURCES) \
-#                                 $(libtcmalloc_la_SOURCES) \
-#                                 $(TCMALLOC_UNITTEST_INCLUDES)
-# tcm_asserts_unittest_CXXFLAGS = $(PTHREAD_CFLAGS) $(AM_CXXFLAGS)
-# tcm_asserts_unittest_LDFLAGS = $(PTHREAD_CFLAGS) $(TCMALLOC_FLAGS)
-# tcm_asserts_unittest_LDADD = $(LIBSPINLOCK) libmaybe_threads.la libstacktrace.la \
-#                              liblogging.la $(PTHREAD_LIBS)
+TESTS += tcm_asserts_unittest
+tcm_asserts_unittest_INCLUDES = src/config_for_unittests.h \
+                             src/gperftools/malloc_extension.h
+tcm_asserts_unittest_SOURCES = src/tests/tcmalloc_unittest.cc \
+                            src/tcmalloc.h \
+                            src/tests/testutil.h src/tests/testutil.cc \
+                            $(TCMALLOC_UNITTEST_INCLUDES)
+tcm_asserts_unittest_CXXFLAGS = $(PTHREAD_CFLAGS) $(AM_CXXFLAGS)
+tcm_asserts_unittest_LDFLAGS = $(PTHREAD_CFLAGS) $(TCMALLOC_FLAGS)
+tcm_asserts_unittest_LDADD = libtcmalloc_with_asserts.la liblogging.la $(PTHREAD_LIBS)
 
 # This makes sure it's safe to link in both tcmalloc and
 # tcmalloc_minimal.  (One would never do this on purpose, but perhaps

src/tcmalloc.cc

@@ -1429,6 +1429,21 @@ static ATTRIBUTE_NOINLINE void do_free_pages(Span* span, void* ptr) {
   Static::pageheap()->Delete(span);
 }
 
+#ifndef NDEBUG
+// note, with sized deletions we have no means to support win32
+// behavior where we detect "not ours" points and delegate them native
+// memory management. This is because nature of sized deletes
+// bypassing addr -> size class checks. So in this validation code we
+// also assume that sized delete is always used with "our" pointers.
+bool ValidateSizeHint(void* ptr, size_t size_hint) {
+  const PageID p = reinterpret_cast<uintptr_t>(ptr) >> kPageShift;
+  Span* span  = Static::pageheap()->GetDescriptor(p);
+  uint32 cl = 0;
+  Static::sizemap()->GetSizeClass(size_hint, &cl);
+  return (span->sizeclass == cl);
+}
+#endif
+
 // Helper for the object deletion (free, delete, etc.).  Inputs:
 //   ptr is object to be freed
 //   invalid_free_fn is a function that gets invoked on certain "bad frees"
@@ -1444,11 +1459,7 @@ void do_free_with_callback(void* ptr,
   const PageID p = reinterpret_cast<uintptr_t>(ptr) >> kPageShift;
   uint32 cl;
 
-#ifndef NO_TCMALLOC_SAMPLES
-  // we only pass size hint when ptr is not page aligned. Which
-  // implies that it must be very small object.
-  ASSERT(!use_hint || size_hint < kPageSize);
-#endif
+  ASSERT(!use_hint || ValidateSizeHint(ptr, size_hint));
 
   if (!use_hint || PREDICT_FALSE(!Static::sizemap()->GetSizeClass(size_hint, &cl))) {
     // if we're in sized delete, but size is too large, no need to

src/tests/tcmalloc_unittest.cc

@@ -1268,6 +1268,23 @@ static int RunAllTests(int argc, char** argv) {
     std::stable_sort(v.begin(), v.end());
   }
 
+#ifdef ENABLE_SIZED_DELETE
+  {
+    fprintf(LOGSTREAM, "Testing large sized delete is not crashing\n");
+    // Large sized delete
+    // case. https://github.com/gperftools/gperftools/issues/1254
+    std::vector<char*> addresses;
+    constexpr int kSizedDepth = 1024;
+    addresses.reserve(kSizedDepth);
+    for (int i = 0; i < kSizedDepth; i++) {
+      addresses.push_back(noopt(new char[12686]));
+    }
+    for (int i = 0; i < kSizedDepth; i++) {
+      ::operator delete[](addresses[i], 12686);
+    }
+  }
+#endif
+
   // Test each of the memory-allocation functions once, just as a sanity-check
   fprintf(LOGSTREAM, "Sanity-testing all the memory allocation functions\n");
   {