8270308: Arena::Amalloc may return misaligned address on 32-bit

Reviewed-by: coleenp, kbarrett

src/hotspot/share/memory/arena.cpp

@@ -30,8 +30,17 @@
 #include "runtime/task.hpp"
 #include "runtime/threadCritical.hpp"
 #include "services/memTracker.hpp"
+#include "utilities/align.hpp"
+#include "utilities/debug.hpp"
 #include "utilities/ostream.hpp"
 
+// Pre-defined default chunk sizes must be arena-aligned, see Chunk::operator new()
+STATIC_ASSERT(is_aligned((int)Chunk::tiny_size, ARENA_AMALLOC_ALIGNMENT));
+STATIC_ASSERT(is_aligned((int)Chunk::init_size, ARENA_AMALLOC_ALIGNMENT));
+STATIC_ASSERT(is_aligned((int)Chunk::medium_size, ARENA_AMALLOC_ALIGNMENT));
+STATIC_ASSERT(is_aligned((int)Chunk::size, ARENA_AMALLOC_ALIGNMENT));
+STATIC_ASSERT(is_aligned((int)Chunk::non_pool_size, ARENA_AMALLOC_ALIGNMENT));
+
 //--------------------------------------------------------------------------------------
 // ChunkPool implementation
 
@@ -171,13 +180,30 @@ class ChunkPoolCleaner : public PeriodicTask {
 //--------------------------------------------------------------------------------------
 // Chunk implementation
 
-void* Chunk::operator new (size_t requested_size, AllocFailType alloc_failmode, size_t length) throw() {
-  // requested_size is equal to sizeof(Chunk) but in order for the arena
-  // allocations to come out aligned as expected the size must be aligned
-  // to expected arena alignment.
-  // expect requested_size but if sizeof(Chunk) doesn't match isn't proper size we must align it.
-  assert(ARENA_ALIGN(requested_size) == aligned_overhead_size(), "Bad alignment");
-  size_t bytes = ARENA_ALIGN(requested_size) + length;
+void* Chunk::operator new (size_t sizeofChunk, AllocFailType alloc_failmode, size_t length) throw() {
+
+  // - requested_size = sizeof(Chunk)
+  // - length = payload size
+  // We must ensure that the boundaries of the payload (C and D) are aligned to 64-bit:
+  //
+  // +-----------+--+--------------------------------------------+
+  // |           |g |                                            |
+  // | Chunk     |a |               Payload                      |
+  // |           |p |                                            |
+  // +-----------+--+--------------------------------------------+
+  // A           B  C                                            D
+  //
+  // - The Chunk is allocated from C-heap, therefore its start address (A) should be
+  //   64-bit aligned on all our platforms, including 32-bit.
+  // - sizeof(Chunk) (B) may not be aligned to 64-bit, and we have to take that into
+  //   account when calculating the Payload bottom (C) (see Chunk::bottom())
+  // - the payload size (length) must be aligned to 64-bit, which takes care of 64-bit
+  //   aligning (D)
+
+  assert(sizeofChunk == sizeof(Chunk), "weird request size");
+  assert(is_aligned(length, ARENA_AMALLOC_ALIGNMENT), "chunk payload length misaligned: "
+         SIZE_FORMAT ".", length);
+  size_t bytes = ARENA_ALIGN(sizeofChunk) + length;
   switch (length) {
    case Chunk::size:        return ChunkPool::large_pool()->allocate(bytes, alloc_failmode);
    case Chunk::medium_size: return ChunkPool::medium_pool()->allocate(bytes, alloc_failmode);
@@ -188,6 +214,8 @@ void* Chunk::operator new (size_t requested_size, AllocFailType alloc_failmode,
      if (p == NULL && alloc_failmode == AllocFailStrategy::EXIT_OOM) {
        vm_exit_out_of_memory(bytes, OOM_MALLOC_ERROR, "Chunk::new");
      }
+     // We rely on arena alignment <= malloc alignment.
+     assert(is_aligned(p, ARENA_AMALLOC_ALIGNMENT), "Chunk start address misaligned.");
      return p;
    }
   }
@@ -239,8 +267,7 @@ void Chunk::start_chunk_pool_cleaner_task() {
 //------------------------------Arena------------------------------------------
 
 Arena::Arena(MEMFLAGS flag, size_t init_size) : _flags(flag), _size_in_bytes(0)  {
-  size_t round_size = (sizeof (char *)) - 1;
-  init_size = (init_size+round_size) & ~round_size;
+  init_size = ARENA_ALIGN(init_size);
   _first = _chunk = new (AllocFailStrategy::EXIT_OOM, init_size) Chunk(init_size);
   _hwm = _chunk->bottom();      // Save the cached hwm, max
   _max = _chunk->top();
@@ -340,7 +367,8 @@ size_t Arena::used() const {
 // Grow a new Chunk
 void* Arena::grow(size_t x, AllocFailType alloc_failmode) {
   // Get minimal required size.  Either real big, or even bigger for giant objs
-  size_t len = MAX2(x, (size_t) Chunk::size);
+  // (Note: all chunk sizes have to be 64-bit aligned)
+  size_t len = MAX2(ARENA_ALIGN(x), (size_t) Chunk::size);
 
   Chunk *k = _chunk;            // Get filled-up chunk address
   _chunk = new (alloc_failmode, len) Chunk(len);

src/hotspot/share/memory/arena.hpp

@@ -52,11 +52,12 @@ class Chunk: CHeapObj<mtChunk> {
   enum {
     // default sizes; make them slightly smaller than 2**k to guard against
     // buddy-system style malloc implementations
+    // Note: please keep these constants 64-bit aligned.
 #ifdef _LP64
     slack      = 40,            // [RGV] Not sure if this is right, but make it
                                 //       a multiple of 8.
 #else
-    slack      = 20,            // suspected sizeof(Chunk) + internal malloc headers
+    slack      = 24,            // suspected sizeof(Chunk) + internal malloc headers
 #endif
 
     tiny_size  =  256  - slack, // Size of first chunk (tiny)
@@ -134,6 +135,11 @@ class Arena : public CHeapObj<mtNone> {
   void* Amalloc(size_t x, AllocFailType alloc_failmode = AllocFailStrategy::EXIT_OOM) {
     x = ARENA_ALIGN(x);  // note for 32 bits this should align _hwm as well.
     debug_only(if (UseMallocOnly) return malloc(x);)
+    // Amalloc guarantees 64-bit alignment and we need to ensure that in case the preceding
+    // allocation was AmallocWords. Only needed on 32-bit - on 64-bit Amalloc and AmallocWords are
+    // identical.
+    assert(is_aligned(_max, ARENA_AMALLOC_ALIGNMENT), "chunk end unaligned?");
+    NOT_LP64(_hwm = ARENA_ALIGN(_hwm));
     return internal_amalloc(x, alloc_failmode);
   }
 

test/hotspot/gtest/memory/test_arena.cpp

@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2021 SAP SE. All rights reserved.
+ *
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include "precompiled.hpp"
+#include "memory/arena.hpp"
+#include "utilities/align.hpp"
+#include "utilities/globalDefinitions.hpp"
+#include "unittest.hpp"
+
+#ifndef LP64
+// These tests below are about alignment issues when mixing Amalloc and AmallocWords.
+// Since on 64-bit these APIs offer the same alignment, they only matter for 32-bit.
+
+TEST_VM(Arena, mixed_alignment_allocation) {
+  // Test that mixed alignment allocations work and provide allocations with the correct
+  // alignment
+  Arena ar(mtTest);
+  void* p1 = ar.AmallocWords(BytesPerWord);
+  void* p2 = ar.Amalloc(BytesPerLong);
+  ASSERT_TRUE(is_aligned(p1, BytesPerWord));
+  ASSERT_TRUE(is_aligned(p2, ARENA_AMALLOC_ALIGNMENT));
+}
+
+TEST_VM(Arena, Arena_with_crooked_initial_size) {
+  // Test that an arena with a crooked, not 64-bit aligned initial size
+  // works
+  Arena ar(mtTest, 4097);
+  void* p1 = ar.AmallocWords(BytesPerWord);
+  void* p2 = ar.Amalloc(BytesPerLong);
+  ASSERT_TRUE(is_aligned(p1, BytesPerWord));
+  ASSERT_TRUE(is_aligned(p2, ARENA_AMALLOC_ALIGNMENT));
+}
+
+TEST_VM(Arena, Arena_grows_large_unaligned) {
+  // Test that if the arena grows with a large unaligned value, nothing bad happens.
+  // We trigger allocation of a new, large, unaligned chunk with a non-standard size
+  // (only possible on 32-bit when allocating with word alignment).
+  // Then we alloc some more. If Arena::grow() does not correctly align, on 32-bit
+  // something should assert at some point.
+  Arena ar(mtTest, 100); // first chunk is small
+  void* p = ar.AmallocWords(Chunk::size + BytesPerWord); // if Arena::grow() misaligns, this asserts
+  // some more allocations for good measure
+  for (int i = 0; i < 100; i ++) {
+    ar.Amalloc(1);
+  }
+}
+
+#endif //  LP64