Bug ID: JDK-8269537 memset() is called after operator new

JDK-8269537 : memset() is called after operator new

Type: Bug
Component: hotspot
Sub-Component: runtime
Affected Version: 17

Priority: P4
Status: Open
Resolution: Unresolved

Submitted: 2021-06-28
Updated: 2024-08-29

Versions (Unresolved/Resolved/Fixed)

The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.

Other
tbdUnresolved

Related Reports

Relates :	JDK-8213481 - [REDO] Fix incorrect copy constructors in hotspot
Relates :	JDK-8264735 - Make dynamic dump repeatable
Relates :	JDK-8231356 - Fix broken ResourceObj::operator new[] in debug builds

Description

Note: this bug can be reproduced in JDK 17u only.
JDK 17u repo: https://github.com/openjdk/jdk17u
gcc version 10.3.0 (GCC) 

For some reason, it's no longer reproducible in JDK mainline. For a simpler reproducer in the JDK mainline, see the second comment below.

The following patch causes a crash in the delete operation:

--- a/src/hotspot/share/classfile/systemDictionaryShared.cpp
+++ b/src/hotspot/share/classfile/systemDictionaryShared.cpp
@@ -1257,6 +1257,8 @@ DumpTimeSharedClassInfo* SystemDictionaryShared::find_or_allocate_info_for_locke
   assert_lock_strong(DumpTimeTable_lock);
   if (_dumptime_table == NULL) {
     _dumptime_table = new (ResourceObj::C_HEAP, mtClass)DumpTimeSharedClassTable();
+    delete _dumptime_table;
+    _dumptime_table = new (ResourceObj::C_HEAP, mtClass)DumpTimeSharedClassTable();
   }
   return _dumptime_table->find_or_allocate_info_for(k, _dump_in_progress);
 }

Gcc generates the following code:

   0x00007ffff6681adc <+14>:	mov    $0x2,%esi
   0x00007ffff6681ae1 <+19>:	mov    $0x1f0a8,%edi
   0x00007ffff6681ae6 <+24>:	callq  0x7ffff5a2e7dc <ResourceObj::operator new(unsigned long, ResourceObj::allocation_type, MEMFLAGS)>
   0x00007ffff6681aeb <+29>:	mov    %rax,%rbx
   0x00007ffff6681aee <+32>:	test   %rbx,%rbx
   0x00007ffff6681af1 <+35>:	je     0x7ffff6681b10 <foofoo()+66>
   0x00007ffff6681af3 <+37>:	mov    %rbx,%rax
   0x00007ffff6681af6 <+40>:	mov    $0x1f0a8,%edx
   0x00007ffff6681afb <+45>:	mov    $0x0,%esi
   0x00007ffff6681b00 <+50>:	mov    %rax,%rdi
>>>0x00007ffff6681b03 <+53>:	callq  0x7ffff5827470 <memset@plt> <<<<<<<<<<<<<<<<< HERE
   0x00007ffff6681b08 <+58>:	mov    %rbx,%rdi
   0x00007ffff6681b0b <+61>:	callq  0x7ffff6687db8 <DumpTimeSharedClassTable::DumpTimeSharedClassTable()>

ResourceObj::_allocation_t is initialized inside the new operator, and it gets trashed by the subsequent call to memset().

As a result, we get an assertion failure inside the destructor:

$ java -Xshare:dump
[....]
#  Internal Error (/jdk2/mnd/open/src/hotspot/share/memory/allocation.cpp:190), pid=10343, tid=10344
#  assert(~(_allocation_t[0] | allocation_mask) == (uintptr_t)this) failed: lost resource object

V  [libjvm.so+0x4ebb89]  ResourceObj::get_allocation_type() const+0x31
V  [libjvm.so+0x4ec0e8]  ResourceObj::allocated_on_C_heap() const+0x18
V  [libjvm.so+0x4eb9f0]  ResourceObj::operator delete(void*)+0x18
V  [libjvm.so+0x113eb17]  SystemDictionaryShared::find_or_allocate_info_for_locked(InstanceKlass*)+0x8b


However, adding a dummy constructor for DumpTimeSharedClassTable makes the problem goes away.  The memset is no longer generated by gcc.

==================
--- a/src/hotspot/share/classfile/systemDictionaryShared.cpp
+++ b/src/hotspot/share/classfile/systemDictionaryShared.cpp
@@ -219,6 +219,7 @@ class DumpTimeSharedClassTable: public ResourceHashtable<
   int _builtin_count;
   int _unregistered_count;
 public:
+  DumpTimeSharedClassTable(int dummy) {}
   DumpTimeSharedClassInfo* find_or_allocate_info_for(InstanceKlass* k, bool dump_in_progress) {
     bool created = false;
     DumpTimeSharedClassInfo* p;
@@ -1256,7 +1257,9 @@ DumpTimeSharedClassInfo* SystemDictionaryShared::find_or_allocate_info_for(Insta
 DumpTimeSharedClassInfo* SystemDictionaryShared::find_or_allocate_info_for_locked(InstanceKlass* k) {
   assert_lock_strong(DumpTimeTable_lock);
   if (_dumptime_table == NULL) {
-    _dumptime_table = new (ResourceObj::C_HEAP, mtClass)DumpTimeSharedClassTable();
+    _dumptime_table = new (ResourceObj::C_HEAP, mtClass)DumpTimeSharedClassTable(0);
+    delete _dumptime_table;
+    _dumptime_table = new (ResourceObj::C_HEAP, mtClass)DumpTimeSharedClassTable(0);
   }
   return _dumptime_table->find_or_allocate_info_for(k, _dump_in_progress);
 }

Comments

I'm unassigning myself off of this ticket. It's unlikely that I'll be able to fix it within the near future, and I have higher priority work.
29-08-2024
I tried forcing subclasses of ResourceObj to declare a non-default constructor by deleting the default constructor of ResourceObj, like this: class GoodResourceObj { protected: GoodResourceObj() = delete; GoodResourceObj(int dummy) ... However, this works only for immediate subclasses. It's still possible to trigger this bug with an indirect subclass. See attatchment https://bugs.openjdk.org/secure/attachment/100472/ioi-test-20220810.zip $ gcc -v [...] gcc version 11.2.0 (Ubuntu 11.2.0-19ubuntu1) $ g++ app.cpp allocation.cpp && ./a.out b1 = new BadBigObj; => 2 b2 = new BadBigObj(); => 1 (should be 2!) b3 = new GoodBigObj; => 2 b4 = new GoodBigObj(); => 2 (should be 2) b5 = new BadBigObj2; => 2 b6 = new BadBigObj2(); => 1 (should be 2!) / class GoodSmallObj : public GoodResourceObj { public: GoodSmallObj() : GoodResourceObj(0) {} }; class BadBigObj2 : public GoodBigObj { int _array[16 1024]; }; BadBigObj2* b5 = new BadBigObj2; BadBigObj2* b6 = new BadBigObj2(); printf("b5 = new BadBigObj2; => %d\n", b5->secret()); printf("b6 = new BadBigObj2(); => %d (should be 2!)\n", b6->secret());
18-08-2022
I can not continue with this bug as we can not reach any conclusion on how to solve it.
09-06-2022
Although I dislike my solution of using thread locals, I think it is a bit better than depending on unspecified object layout in this way.
23-05-2022
Yes, vtables are problematic, but ResourceObj already has a vtable. Also, having a vtable would affect the existing implementation of ResourceObj::set_allocation_type as well. ==================== #define ALLOCATION_SUPER_CLASS_SPEC : public AllocatedObj class AllocatedObj { public: // Printing support void print() const; void print_value() const; virtual void print_on(outputStream* st) const; virtual void print_value_on(outputStream* st) const; }; class ResourceObj ALLOCATION_SUPER_CLASS_SPEC { ....
17-05-2022
And also without multiple inheritance: #include <iostream> using namespace std; class Resource { int y; public: Resource () { cout << "Resource: size = " << sizeof(this) << " @" << this << endl; } }; class Test : Resource { int y; virtual void test() {}; public: Test () { cout << "Test: size = " << sizeof(this) << " @" << this << endl; } }; int main() { Test t; cout << "Outside Test: size = " << sizeof(t) << " @" << &t << endl; } Resource: size = 4 @0x7ffebef19738 Test: size = 16 @0x7ffebef19730 Outside Test: size = 16 @0x7ffebef19730
17-05-2022
and if you flip to class Test : Resource, VirtualBase { Resource: size = 4 @0x7ffc19b19e54 Test: size = 24 @0x7ffc19b19e48 Outside Test: size = 24 @0x7ffc19b19e48
17-05-2022
#include <iostream> using namespace std; class VirtualBase { int x; virtual void test() = 0; }; class Resource { int y; public: Resource () { cout << "Resource: size = " << sizeof(this) << " @" << this << endl; } }; class Test : VirtualBase, Resource { int y; void test() override {}; public: Test () { cout << "Test: size = " << sizeof(this) << " @" << this << endl; } }; int main() { Test t; cout << "Outside Test: size = " << sizeof(t) << " @" << &t << endl; } Rsource: size = 4 @0x7ffcc977be64 Test: size = 24 @0x7ffcc977be58 Outside Test: size = 24 @0x7ffcc977be58
17-05-2022
And if you have a vtable?
17-05-2022
Effectively my proposal moves this field from inside of ResourceObj to outside, so they won't be zapped during the initialization of ResourceObj: class ResourceObj ALLOCATION_SUPER_CLASS_SPEC { #ifdef ASSERT private: uintptr_t _allocation_t[2]; <<<< HERE
17-05-2022
I did an experiment with gcc. The base classes are laid out in the order they appear in the subclass's declaration. So -- putting the alloc_info as a prefix won't work with Sub2 in the code below, but neither does the current HotSpot code, which also assumes a fixed distance of the malloc buffer to a field inside the ResourceObj. This means that the alloc_info prefix is not a regression :-) ===== #include <new> #include <iostream> #include "stdlib.h" class A { int a; public: A() { std::cout << "A: @" << this << "\n"; } void* operator new(size_t size) { void* p = malloc(size); std::cout << "operator A::new(), size = " << size << " @ " << p << "\n"; return p; } }; class B { int b; public: B() { std::cout << "B: @" << this << "\n"; } }; class Sub1 : public A, public B { public: Sub1() { std::cout << "Sub1: size = " << sizeof(this) << " @" << this << "\n"; } }; class Sub2 : public B, public A { public: Sub2() { std::cout << "Sub2: size = " << sizeof(this) << " @" << this << "\n"; } }; int main() { Sub1 s1 = new Sub1(); Sub2 s2 = new Sub2(); }; ===== $ g++ multi.cpp && ./a.out operator A::new(), size = 8 @ 0x231deb0 A: @0x231deb0 B: @0x231deb4 Sub1: size = 8 @0x231deb0 operator A::new(), size = 8 @ 0x231e2e0 B: @0x231e2e0 A: @0x231e2e4 Sub2: size = 8 @0x231e2e0 $ g++ -v [...] gcc version 10.3.0 (GCC)
17-05-2022
From my understanding you want to read memory in front of the object to get the allocation type, I guess you get the address by subtracting from the `this`pointer. However where does this point, is it well defined in c++ at all? In the case of multiple inheritance consider an object A inheriting from B and C: ``` \|-alloc info-\|-----B----\|---C--- \| \|---------------\|----------A---------\| ``` What is `this` (in the ResourceObj) pointing to if B is ResourceObj, and what is `this` pointing to if C is ResourceObj? What should the offset be?
17-05-2022
[~lkorinth] [~kbarrett] could you give an example to show why multiple inheritance would be a problem?
16-05-2022
I have discussed that with Kim I believe, and if I remember correctly, the problem is that you can not know the offset to the allocation type when using multiple inheritance.
16-05-2022
I have another idea: - Do not store allocation type in the ResourceObj anymore. - In the "operator new()" variants, allocate a padding block in front of the ResourceObj. Store the allocation type there along with a marker. - get_allocation_type() reads the padding block. If the marker doesn't exist, it's STACK_OR_EMBEDDED. There is a random chance that a stack-allocated ResourceObj happens to have the marker, but this problem also exists in the following code today, so we don't have a regression: void ResourceObj::initialize_allocation_info() { if (~(_allocation_t[0] \| allocation_mask) != (uintptr_t)this) { // Operator new() is not called for allocations // on stack and for embedded objects. set_allocation_type((address)this, STACK_OR_EMBEDDED); Note: for architectures with a downward growing stack, it's always safe to read the padding block. For upward growing stacks, maybe we can use SafeFetch if (this - size_padding_block) would cross a page boundary?? Not sure how this would interfere with the stack banging code, though. One option is to always disable the allocation type check on upward growing stacks. x86/x64 are always downward growing. ARM can be configured to grow upward, but I doubt anyone is using it.
13-05-2022
I want to remove tracking of allocation type or fix it according to pull request (not optimal) or take some other action, but I get no feedback
13-05-2022
I think I might have a fix for this
03-09-2021
So I'm not sure there's anything to be done under the rubric of "memset() is called after operator new". That's just how the language works. I've spent a little time trying to come up with a way to detect whether usage of the kind described would involve zero-initialization (so that at least some uses of resource allocation could detect and complain), but haven't come up with anything yet and think it might not be possible. (But never say never in C++, as there are lots of dark corners.) Maybe all that's needed is to backport JDK-8264735 (or some part of it) to jdk17?
02-09-2021
XXX* xxx = new (ResourceObj::C_HEAP, mtClass) XXX; is specified to default-initialize the object (C++14 5.3.4/17, 8.5/11) XXX* xxx = new (ResourceObj::C_HEAP, mtClass) XXX(); is specified to value-initialize the object (C++14 8.5/12, 8.5/17) If a class has no user-defined or deleted default constructor and is value-initialized then it is first zero-initialized and then default constructed if the default constructor is not trivial. (C++14 8.5/8) (A class can have no user-defined default constructor but still have the default constructor be non-trivial because of a user-defined default constructor in a base class.) Value-initialization when there is a user-defined constructor is default-initialization. (C++14 8.5 8.5/8) So to avoid the memset you either need to not value-initialize, or ensure the class has a user-defined default constructor. OTOH, the whole scheme of passing information from the allocator that way is UB for other reasons that I don't feel like chasing down right now. This is mentioned in the comment preceding ResourceObj::initialize_allocation_info (JDK-8213481). This is also related to JDK-8231356. The problem class from the bug description (DumpTimeSharedClassTable) doesn't have a user-defined default constructor in jdk17, but does in jdk18 (JDK-8264735).
02-09-2021
Apparently, the memset is generated only if you do this: XXX* xxx = new (ResourceObj::C_HEAP, mtClass) XXX(); but not this (no ()) in the new : XXX* xxx = new (ResourceObj::C_HEAP, mtClass) XXX;
02-09-2021
Here's a simple reproducer: ================================== diff --git a/src/hotspot/share/classfile/systemDictionaryShared.cpp b/src/hotspot/share/classfile/systemDictionaryShared.cpp index 9d1e378369d..a23b98ef895 100644 --- a/src/hotspot/share/classfile/systemDictionaryShared.cpp +++ b/src/hotspot/share/classfile/systemDictionaryShared.cpp @@ -192,10 +192,25 @@ DumpTimeClassInfo* SystemDictionaryShared::find_or_allocate_info_for(InstanceKla return find_or_allocate_info_for_locked(k); } +class FooBar : public ResourceObj { + int dummy[1000000]; + +public: + FooBar() { memset(dummy, 0, sizeof(dummy)); } + ~FooBar() {} +}; + +class XXX : public FooBar { + int a; + int b; +}; + DumpTimeClassInfo* SystemDictionaryShared::find_or_allocate_info_for_locked(InstanceKlass* k) { assert_lock_strong(DumpTimeTable_lock); if (_dumptime_table == NULL) { _dumptime_table = new (ResourceObj::C_HEAP, mtClass) DumpTimeSharedClassTable; + XXX* xxx = new (ResourceObj::C_HEAP, mtClass) XXX(); + delete xxx; } return _dumptime_table->find_or_allocate_info_for(k, _dump_in_progress); } ================================== $ java -Xshare:dump # To suppress the following error report, specify this argument # after -XX: or in .hotspotrc: SuppressErrorAt=/allocation.cpp:190 # # A fatal error has been detected by the Java Runtime Environment: # # Internal Error (/jdk2/nep/open/src/hotspot/share/memory/allocation.cpp:190), pid=13164, tid=13165 # assert(~(_allocation_t[0] \| allocation_mask) == (uintptr_t)this) failed: lost resource object # # JRE version: (18.0) (slowdebug build ) # Java VM: Java HotSpot(TM) 64-Bit Server VM (slowdebug 18-internal+0-adhoc.iklam.open, interpreted mode, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64) # Problematic frame: # V [libjvm.so+0x4ed601] ResourceObj::get_allocation_type() const+0x31 #
02-09-2021
Does the c++ spec say anything about whether the contents of the memory allocated inside the "operator new" must be preserved before the constructor is called? Our code has this assumption.
28-06-2021