8275320: NMT should perform buffer overrun checks

8275320: NMT should perform buffer overrun checks
8275301: Unify C-heap buffer overrun checks into NMT

Reviewed-by: simonis, zgu

Author: tstuefe

src/hotspot/share/runtime/os.cpp

@@ -669,13 +669,14 @@ void* os::malloc(size_t size, MEMFLAGS memflags, const NativeCallStack& stack) {
 
   // NMT support
   NMT_TrackingLevel level = MemTracker::tracking_level();
-  size_t            nmt_header_size = MemTracker::malloc_header_size(level);
+  const size_t nmt_overhead =
+      MemTracker::malloc_header_size(level) + MemTracker::malloc_footer_size(level);
 
 #ifndef ASSERT
-  const size_t alloc_size = size + nmt_header_size;
+  const size_t alloc_size = size + nmt_overhead;
 #else
-  const size_t alloc_size = GuardedMemory::get_total_size(size + nmt_header_size);
-  if (size + nmt_header_size > alloc_size) { // Check for rollover.
+  const size_t alloc_size = GuardedMemory::get_total_size(size + nmt_overhead);
+  if (size + nmt_overhead > alloc_size) { // Check for rollover.
     return NULL;
   }
 #endif
@@ -693,7 +694,7 @@ void* os::malloc(size_t size, MEMFLAGS memflags, const NativeCallStack& stack) {
     return NULL;
   }
   // Wrap memory with guard
-  GuardedMemory guarded(ptr, size + nmt_header_size);
+  GuardedMemory guarded(ptr, size + nmt_overhead);
   ptr = guarded.get_user_ptr();
 
   if ((intptr_t)ptr == (intptr_t)MallocCatchPtr) {
@@ -741,8 +742,9 @@ void* os::realloc(void *memblock, size_t size, MEMFLAGS memflags, const NativeCa
    // NMT support
   NMT_TrackingLevel level = MemTracker::tracking_level();
   void* membase = MemTracker::record_free(memblock, level);
-  size_t  nmt_header_size = MemTracker::malloc_header_size(level);
-  void* ptr = ::realloc(membase, size + nmt_header_size);
+  const size_t nmt_overhead =
+      MemTracker::malloc_header_size(level) + MemTracker::malloc_footer_size(level);
+  void* ptr = ::realloc(membase, size + nmt_overhead);
   return MemTracker::record_malloc(ptr, size, memflags, stack, level);
 #else
   if (memblock == NULL) {
@@ -761,7 +763,10 @@ void* os::realloc(void *memblock, size_t size, MEMFLAGS memflags, const NativeCa
   if (ptr != NULL ) {
     GuardedMemory guarded(MemTracker::malloc_base(memblock));
     // Guard's user data contains NMT header
-    size_t memblock_size = guarded.get_user_size() - MemTracker::malloc_header_size(memblock);
+    NMT_TrackingLevel level = MemTracker::tracking_level();
+    const size_t nmt_overhead =
+        MemTracker::malloc_header_size(level) + MemTracker::malloc_footer_size(level);
+    size_t memblock_size = guarded.get_user_size() - nmt_overhead;
     memcpy(ptr, memblock, MIN2(size, memblock_size));
     if (paranoid) {
       verify_memory(MemTracker::malloc_base(ptr));

src/hotspot/share/services/mallocSiteTable.cpp

@@ -39,7 +39,6 @@ volatile int MallocSiteTable::_access_count = 0;
 // Tracking hashtable contention
 NOT_PRODUCT(int MallocSiteTable::_peak_count = 0;)
 
-
 /*
  * Initialize malloc site table.
  * Hashtable entry is malloc'd, so it can cause infinite recursion.
@@ -49,7 +48,6 @@ NOT_PRODUCT(int MallocSiteTable::_peak_count = 0;)
  * time, it is in single-threaded mode from JVM perspective.
  */
 bool MallocSiteTable::initialize() {
-  assert((size_t)table_size <= MAX_MALLOCSITE_TABLE_SIZE, "Hashtable overflow");
 
   // Fake the call stack for hashtable entry allocation
   assert(NMT_TrackingStackDepth > 1, "At least one tracking stack");

src/hotspot/share/services/mallocSiteTable.hpp

@@ -114,6 +114,9 @@ class MallocSiteTable : AllStatic {
     table_size = (table_base_size * NMT_TrackingStackDepth - 1)
   };
 
+  // The table must not be wider than the maximum value the bucket_idx field
+  // in the malloc header can hold.
+  STATIC_ASSERT(table_size <= MAX_MALLOCSITE_TABLE_SIZE);
 
   // This is a very special lock, that allows multiple shared accesses (sharedLock), but
   // once exclusive access (exclusiveLock) is requested, all shared accesses are

src/hotspot/share/services/mallocTracker.cpp

@@ -23,10 +23,13 @@
  */
 #include "precompiled.hpp"
 
+#include "runtime/os.hpp"
 #include "services/mallocSiteTable.hpp"
 #include "services/mallocTracker.hpp"
 #include "services/mallocTracker.inline.hpp"
 #include "services/memTracker.hpp"
+#include "utilities/debug.hpp"
+#include "utilities/ostream.hpp"
 
 size_t MallocMemorySummary::_snapshot[CALC_OBJ_SIZE_IN_TYPE(MallocMemorySnapshot, size_t)];
 
@@ -103,15 +106,118 @@ void MallocMemorySummary::initialize() {
   ::new ((void*)_snapshot)MallocMemorySnapshot();
 }
 
-void MallocHeader::release() const {
+void MallocHeader::mark_block_as_dead() {
+  _canary = _header_canary_dead_mark;
+  NOT_LP64(_alt_canary = _header_alt_canary_dead_mark);
+  set_footer(_footer_canary_dead_mark);
+}
+
+void MallocHeader::release() {
   // Tracking already shutdown, no housekeeping is needed anymore
   if (MemTracker::tracking_level() <= NMT_minimal) return;
 
+  check_block_integrity();
+
   MallocMemorySummary::record_free(size(), flags());
   MallocMemorySummary::record_free_malloc_header(sizeof(MallocHeader));
   if (MemTracker::tracking_level() == NMT_detail) {
     MallocSiteTable::deallocation_at(size(), _bucket_idx, _pos_idx);
   }
+
+  mark_block_as_dead();
+}
+
+void MallocHeader::print_block_on_error(outputStream* st, address bad_address) const {
+  assert(bad_address >= (address)this, "sanity");
+
+  // This function prints block information, including hex dump, in case of a detected
+  // corruption. The hex dump should show both block header and corruption site
+  // (which may or may not be close together or identical). Plus some surrounding area.
+  //
+  // Note that we use os::print_hex_dump(), which is able to cope with unmapped
+  // memory (it uses SafeFetch).
+
+  st->print_cr("NMT Block at " PTR_FORMAT ", corruption at: " PTR_FORMAT ": ",
+               p2i(this), p2i(bad_address));
+  static const size_t min_dump_length = 256;
+  address from1 = align_down((address)this, sizeof(void*)) - (min_dump_length / 2);
+  address to1 = from1 + min_dump_length;
+  address from2 = align_down(bad_address, sizeof(void*)) - (min_dump_length / 2);
+  address to2 = from2 + min_dump_length;
+  if (from2 > to1) {
+    // Dump gets too large, split up in two sections.
+    os::print_hex_dump(st, from1, to1, 1);
+    st->print_cr("...");
+    os::print_hex_dump(st, from2, to2, 1);
+  } else {
+    // print one hex dump
+    os::print_hex_dump(st, from1, to2, 1);
+  }
+}
+
+// Check block integrity. If block is broken, print out a report
+// to tty (optionally with hex dump surrounding the broken block),
+// then trigger a fatal error.
+void MallocHeader::check_block_integrity() const {
+
+#define PREFIX "NMT corruption: "
+  // Note: if you modify the error messages here, make sure you
+  // adapt the associated gtests too.
+
+  // Weed out obviously wrong block addresses of NULL or very low
+  // values. Note that we should not call this for ::free(NULL),
+  // which should be handled by os::free() above us.
+  if (((size_t)p2i(this)) < K) {
+    fatal(PREFIX "Block at " PTR_FORMAT ": invalid block address", p2i(this));
+  }
+
+  // From here on we assume the block pointer to be valid. We could
+  // use SafeFetch but since this is a hot path we don't. If we are
+  // wrong, we will crash when accessing the canary, which hopefully
+  // generates distinct crash report.
+
+  // Weed out obviously unaligned addresses. NMT blocks, being the result of
+  // malloc calls, should adhere to malloc() alignment. Malloc alignment is
+  // specified by the standard by this requirement:
+  // "malloc returns a pointer which is suitably aligned for any built-in type"
+  // For us it means that it is *at least* 64-bit on all of our 32-bit and
+  // 64-bit platforms since we have native 64-bit types. It very probably is
+  // larger than that, since there exist scalar types larger than 64bit. Here,
+  // we test the smallest alignment we know.
+  // Should we ever start using std::max_align_t, this would be one place to
+  // fix up.
+  if (!is_aligned(this, sizeof(uint64_t))) {
+    print_block_on_error(tty, (address)this);
+    fatal(PREFIX "Block at " PTR_FORMAT ": block address is unaligned", p2i(this));
+  }
+
+  // Check header canary
+  if (_canary != _header_canary_life_mark) {
+    print_block_on_error(tty, (address)this);
+    fatal(PREFIX "Block at " PTR_FORMAT ": header canary broken.", p2i(this));
+  }
+
+#ifndef _LP64
+  // On 32-bit we have a second canary, check that one too.
+  if (_alt_canary != _header_alt_canary_life_mark) {
+    print_block_on_error(tty, (address)this);
+    fatal(PREFIX "Block at " PTR_FORMAT ": header alternate canary broken.", p2i(this));
+  }
+#endif
+
+  // Does block size seems reasonable?
+  if (_size >= max_reasonable_malloc_size) {
+    print_block_on_error(tty, (address)this);
+    fatal(PREFIX "Block at " PTR_FORMAT ": header looks invalid (weirdly large block size)", p2i(this));
+  }
+
+  // Check footer canary
+  if (get_footer() != _footer_canary_life_mark) {
+    print_block_on_error(tty, footer_address());
+    fatal(PREFIX "Block at " PTR_FORMAT ": footer canary broken at " PTR_FORMAT " (buffer overflow?)",
+          p2i(this), p2i(footer_address()));
+  }
+#undef PREFIX
 }
 
 bool MallocHeader::record_malloc_site(const NativeCallStack& stack, size_t size,

src/hotspot/share/services/mallocTracker.hpp

@@ -239,31 +239,99 @@ class MallocMemorySummary : AllStatic {
 
 /*
  * Malloc tracking header.
- * To satisfy malloc alignment requirement, NMT uses 2 machine words for tracking purpose,
- * which ensures 8-bytes alignment on 32-bit systems and 16-bytes on 64-bit systems (Product build).
+ *
+ * If NMT is active (state >= minimal), we need to track allocations. A simple and cheap way to
+ * do this is by using malloc headers.
+ *
+ * The user allocation is preceded by a header and is immediately followed by a (possibly unaligned)
+ *  footer canary:
+ *
+ * +--------------+-------------  ....  ------------------+-----+
+ * |    header    |               user                    | can |
+ * |              |             allocation                | ary |
+ * +--------------+-------------  ....  ------------------+-----+
+ *     16 bytes              user size                      2 byte
+ *
+ * Alignment:
+ *
+ * The start of the user allocation needs to adhere to malloc alignment. We assume 128 bits
+ * on both 64-bit/32-bit to be enough for that. So the malloc header is 16 bytes long on both
+ * 32-bit and 64-bit.
+ *
+ * Layout on 64-bit:
+ *
+ *     0        1        2        3        4        5        6        7
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * |                            64-bit size                                |  ...
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ *
+ *           8        9        10       11       12       13       14       15          16 ++
+ *       +--------+--------+--------+--------+--------+--------+--------+--------+  ------------------------
+ *  ...  |   bucket idx    |     pos idx     | flags  | unused |     canary      |  ... User payload ....
+ *       +--------+--------+--------+--------+--------+--------+--------+--------+  ------------------------
+ *
+ * Layout on 32-bit:
+ *
+ *     0        1        2        3        4        5        6        7
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * |            alt. canary            |           32-bit size             |  ...
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ *
+ *           8        9        10       11       12       13       14       15          16 ++
+ *       +--------+--------+--------+--------+--------+--------+--------+--------+  ------------------------
+ *  ...  |   bucket idx    |     pos idx     | flags  | unused |     canary      |  ... User payload ....
+ *       +--------+--------+--------+--------+--------+--------+--------+--------+  ------------------------
+ *
+ * Notes:
+ * - We have a canary in the two bytes directly preceding the user payload. That allows us to
+ *   catch negative buffer overflows.
+ * - On 32-bit, due to the smaller size_t, we have some bits to spare. So we also have a second
+ *   canary at the very start of the malloc header (generously sized 32 bits).
+ * - The footer canary consists of two bytes. Since the footer location may be unaligned to 16 bits,
+ *   the bytes are stored individually.
  */
 
 class MallocHeader {
-#ifdef _LP64
-  size_t           _size      : 64;
-  size_t           _flags     : 8;
-  size_t           _pos_idx   : 16;
-  size_t           _bucket_idx: 40;
-#define MAX_MALLOCSITE_TABLE_SIZE right_n_bits(40)
-#define MAX_BUCKET_LENGTH         right_n_bits(16)
-#else
-  size_t           _size      : 32;
-  size_t           _flags     : 8;
-  size_t           _pos_idx   : 8;
-  size_t           _bucket_idx: 16;
-#define MAX_MALLOCSITE_TABLE_SIZE  right_n_bits(16)
-#define MAX_BUCKET_LENGTH          right_n_bits(8)
-#endif  // _LP64
+
+  NOT_LP64(uint32_t _alt_canary);
+  size_t _size;
+  uint16_t _bucket_idx;
+  uint16_t _pos_idx;
+  uint8_t _flags;
+  uint8_t _unused;
+  uint16_t _canary;
+
+#define MAX_MALLOCSITE_TABLE_SIZE (USHRT_MAX - 1)
+#define MAX_BUCKET_LENGTH         (USHRT_MAX - 1)
+
+  static const uint16_t _header_canary_life_mark = 0xE99E;
+  static const uint16_t _header_canary_dead_mark = 0xD99D;
+  static const uint16_t _footer_canary_life_mark = 0xE88E;
+  static const uint16_t _footer_canary_dead_mark = 0xD88D;
+  NOT_LP64(static const uint32_t _header_alt_canary_life_mark = 0xE99EE99E;)
+  NOT_LP64(static const uint32_t _header_alt_canary_dead_mark = 0xD88DD88D;)
+
+  // We discount sizes larger than these
+  static const size_t max_reasonable_malloc_size = LP64_ONLY(256 * G) NOT_LP64(3500 * M);
+
+  // Check block integrity. If block is broken, print out a report
+  // to tty (optionally with hex dump surrounding the broken block),
+  // then trigger a fatal error.
+  void check_block_integrity() const;
+  void print_block_on_error(outputStream* st, address bad_address) const;
+  void mark_block_as_dead();
+
+  static uint16_t build_footer(uint8_t b1, uint8_t b2) { return ((uint16_t)b1 << 8) | (uint16_t)b2; }
+
+  uint8_t* footer_address() const   { return ((address)this) + sizeof(MallocHeader) + _size; }
+  uint16_t get_footer() const       { return build_footer(footer_address()[0], footer_address()[1]); }
+  void set_footer(uint16_t v)       { footer_address()[0] = v >> 8; footer_address()[1] = (uint8_t)v; }
 
  public:
+
   MallocHeader(size_t size, MEMFLAGS flags, const NativeCallStack& stack, NMT_TrackingLevel level) {
-    assert(sizeof(MallocHeader) == sizeof(void*) * 2,
-      "Wrong header size");
+
+    assert(size < max_reasonable_malloc_size, "Too large allocation size?");
 
     if (level == NMT_minimal) {
       return;
@@ -277,11 +345,18 @@ class MallocHeader {
       if (record_malloc_site(stack, size, &bucket_idx, &pos_idx, flags)) {
         assert(bucket_idx <= MAX_MALLOCSITE_TABLE_SIZE, "Overflow bucket index");
         assert(pos_idx <= MAX_BUCKET_LENGTH, "Overflow bucket position index");
-        _bucket_idx = bucket_idx;
-        _pos_idx = pos_idx;
+        _bucket_idx = (uint16_t)bucket_idx;
+        _pos_idx = (uint16_t)pos_idx;
       }
     }
 
+    _unused = 0;
+    _canary = _header_canary_life_mark;
+    // On 32-bit we have some bits more, use them for a second canary
+    // guarding the start of the header.
+    NOT_LP64(_alt_canary = _header_alt_canary_life_mark;)
+    set_footer(_footer_canary_life_mark); // set after initializing _size
+
     MallocMemorySummary::record_malloc(size, flags);
     MallocMemorySummary::record_new_malloc_header(sizeof(MallocHeader));
   }
@@ -290,8 +365,8 @@ class MallocHeader {
   inline MEMFLAGS flags() const { return (MEMFLAGS)_flags; }
   bool get_stack(NativeCallStack& stack) const;
 
-  // Cleanup tracking information before the memory is released.
-  void release() const;
+  // Cleanup tracking information and mark block as dead before the memory is released.
+  void release();
 
  private:
   inline void set_size(size_t size) {
@@ -301,6 +376,9 @@ class MallocHeader {
     size_t* bucket_idx, size_t* pos_idx, MEMFLAGS flags) const;
 };
 
+// This needs to be true on both 64-bit and 32-bit platforms
+STATIC_ASSERT(sizeof(MallocHeader) == (sizeof(uint64_t) * 2));
+
 
 // Main class called from MemTracker to track malloc activities
 class MallocTracker : AllStatic {
@@ -315,6 +393,11 @@ class MallocTracker : AllStatic {
     return (level == NMT_off) ? 0 : sizeof(MallocHeader);
   }
 
+  // malloc tracking footer size for specific tracking level
+  static inline size_t malloc_footer_size(NMT_TrackingLevel level) {
+    return (level == NMT_off) ? 0 : sizeof(uint16_t);
+  }
+
   // Parameter name convention:
   // memblock :   the beginning address for user data
   // malloc_base: the beginning address that includes malloc tracking header
@@ -349,11 +432,6 @@ class MallocTracker : AllStatic {
     return header->flags();
   }
 
-  // Get header size
-  static inline size_t get_header_size(void* memblock) {
-    return (memblock == NULL) ? 0 : sizeof(MallocHeader);
-  }
-
   static inline void record_new_arena(MEMFLAGS flags) {
     MallocMemorySummary::record_new_arena(flags);
   }