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 #1254

4 files changed, 81 insertions, 35 deletions

diff --git a/.gitignore b/.gitignore
index 992c8d8..73a1f7c 100644
--- a/.gitignore
+++ b/.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
diff --git a/Makefile.am b/Makefile.am
index d93300d..bc61f62 100644
--- a/Makefile.am
+++ b/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
diff --git a/src/tcmalloc.cc b/src/tcmalloc.cc
index 6d3929d..0fa880c 100644
--- a/src/tcmalloc.cc
+++ b/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
diff --git a/src/tests/tcmalloc_unittest.cc b/src/tests/tcmalloc_unittest.cc
index af47a4c..f1bc078 100644
--- a/src/tests/tcmalloc_unittest.cc
+++ b/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");
   {