RepoMirrors
/
gperftools
mirror of https://github.com/gperftools/gperftools
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Aliaksey Kandratsenka 2021-02-28 15:42:00 -08:00
parent 47b5b59ca9
commit c939dd5531
4 changed files with 81 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.gitignore vendored
View File

@ -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

76
Makefile.am
View File

@ -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

21
src/tcmalloc.cc
View File

@ -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

17
src/tests/tcmalloc_unittest.cc
View File

@ -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");
{