MemoryCleaner race condition and memory corruption after running yieldOwnership #222

Closed

opened 2025-05-08 19:47:52 +02:00 by BwackNinja · 1 comment

BwackNinja commented

2025-05-08 19:47:52 +02:00

(Migrated from github.com)

When MemoryCleaner.yieldOwnership is run by bindings, the entry in Cache remains and the cleaner will still be run. For a short-lived object, a new object can be created and have the same address but be of a different type. From there, when the Cleaner runs, it can attempt to clean up the new object with the incorrect cleanup function, resulting in a crash.

Current thread (0x00007cf86d73f4f0):  JavaThread "Cleaner-1" daemon [_thread_in_native, id=239286, stack(0x00007cf768066000,0x00007cf768166000) (1024K)]

Stack: [0x00007cf768066000,0x00007cf768166000],  sp=0x00007cf768164558,  free space=1017k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
C  0x00007cf86cfdfbc0
v  ~RuntimeStub::nep_invoker_blob 0x00007cf85bb6b89d
J 4769 c1 io.github.jwharm.javagi.interop.MemoryCleaner$StructFinalizer.run()V (147 bytes) @ 0x00007cf854ac4424 [0x00007cf854ac23a0+0x0000000000002084]
J 5051 c2 jdk.internal.ref.PhantomCleanable.clean()V java.base@24.0.1 (20 bytes) @ 0x00007cf85c2724e4 [0x00007cf85c2721e0+0x0000000000000304]
j  jdk.internal.ref.CleanerImpl.run()V+57 java.base@24.0.1
j  java.lang.Thread.runWith(Ljava/lang/Object;Ljava/lang/Runnable;)V+5 java.base@24.0.1
j  java.lang.Thread.run()V+19 java.base@24.0.1
j  jdk.internal.misc.InnocuousThread.run()V+20 java.base@24.0.1
v  ~StubRoutines::call_stub 0x00007cf85b85fc86
V  [libjvm.so+0x587b80]
V  [libjvm.so+0x588782]
V  [libjvm.so+0x64741f]
V  [libjvm.so+0x59d9b5]
V  [libjvm.so+0x8784a6]
C  [libc.so.6+0x957eb]
Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
v  ~RuntimeStub::nep_invoker_blob 0x00007cf85bb6b864
J 4769 c1 io.github.jwharm.javagi.interop.MemoryCleaner$StructFinalizer.run()V (147 bytes) @ 0x00007cf854ac4424 [0x00007cf854ac23a0+0x0000000000002084]
J 5051 c2 jdk.internal.ref.PhantomCleanable.clean()V java.base@24.0.1 (20 bytes) @ 0x00007cf85c2724e4 [0x00007cf85c2721e0+0x0000000000000304]
j  jdk.internal.ref.CleanerImpl.run()V+57 java.base@24.0.1
j  java.lang.Thread.runWith(Ljava/lang/Object;Ljava/lang/Runnable;)V+5 java.base@24.0.1
j  java.lang.Thread.run()V+19 java.base@24.0.1
j  jdk.internal.misc.InnocuousThread.run()V+20 java.base@24.0.1
v  ~StubRoutines::call_stub 0x00007cf85b85fc86

Instead, modify yieldOwnership to only create the entry in Cache if it doesn't already exist, and run the cleanable to both remove the entry from Cache and ensure that the cleanable will not run again.

    public static void yieldOwnership(@NotNull Proxy proxy) {
        requireNonNull(proxy);
        synchronized (cache) {
            Cached cached = cache.get(proxy.handle());
            if (cached != null) {
                cache.put(proxy.handle(), new Cached(false,
                                                     cached.freeFunc,
                                                     cached.boxedType,
                                                     cached.cleanable));
                cached.cleanable.clean();
            }
        }
    }

I can make a pull request if desired.

When `MemoryCleaner.yieldOwnership` is run by bindings, the entry in `Cache` remains and the cleaner will still be run. For a short-lived object, a new object can be created and have the same address but be of a different type. From there, when the Cleaner runs, it can attempt to clean up the new object with the incorrect cleanup function, resulting in a crash. ``` Current thread (0x00007cf86d73f4f0): JavaThread "Cleaner-1" daemon [_thread_in_native, id=239286, stack(0x00007cf768066000,0x00007cf768166000) (1024K)] Stack: [0x00007cf768066000,0x00007cf768166000], sp=0x00007cf768164558, free space=1017k Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code) C 0x00007cf86cfdfbc0 v ~RuntimeStub::nep_invoker_blob 0x00007cf85bb6b89d J 4769 c1 io.github.jwharm.javagi.interop.MemoryCleaner$StructFinalizer.run()V (147 bytes) @ 0x00007cf854ac4424 [0x00007cf854ac23a0+0x0000000000002084] J 5051 c2 jdk.internal.ref.PhantomCleanable.clean()V java.base@24.0.1 (20 bytes) @ 0x00007cf85c2724e4 [0x00007cf85c2721e0+0x0000000000000304] j jdk.internal.ref.CleanerImpl.run()V+57 java.base@24.0.1 j java.lang.Thread.runWith(Ljava/lang/Object;Ljava/lang/Runnable;)V+5 java.base@24.0.1 j java.lang.Thread.run()V+19 java.base@24.0.1 j jdk.internal.misc.InnocuousThread.run()V+20 java.base@24.0.1 v ~StubRoutines::call_stub 0x00007cf85b85fc86 V [libjvm.so+0x587b80] V [libjvm.so+0x588782] V [libjvm.so+0x64741f] V [libjvm.so+0x59d9b5] V [libjvm.so+0x8784a6] C [libc.so.6+0x957eb] Java frames: (J=compiled Java code, j=interpreted, Vv=VM code) v ~RuntimeStub::nep_invoker_blob 0x00007cf85bb6b864 J 4769 c1 io.github.jwharm.javagi.interop.MemoryCleaner$StructFinalizer.run()V (147 bytes) @ 0x00007cf854ac4424 [0x00007cf854ac23a0+0x0000000000002084] J 5051 c2 jdk.internal.ref.PhantomCleanable.clean()V java.base@24.0.1 (20 bytes) @ 0x00007cf85c2724e4 [0x00007cf85c2721e0+0x0000000000000304] j jdk.internal.ref.CleanerImpl.run()V+57 java.base@24.0.1 j java.lang.Thread.runWith(Ljava/lang/Object;Ljava/lang/Runnable;)V+5 java.base@24.0.1 j java.lang.Thread.run()V+19 java.base@24.0.1 j jdk.internal.misc.InnocuousThread.run()V+20 java.base@24.0.1 v ~StubRoutines::call_stub 0x00007cf85b85fc86 ``` Instead, modify `yieldOwnership` to only create the entry in `Cache` if it doesn't already exist, and run the `cleanable` to both remove the entry from `Cache` and ensure that the `cleanable` will not run again. ``` public static void yieldOwnership(@NotNull Proxy proxy) { requireNonNull(proxy); synchronized (cache) { Cached cached = cache.get(proxy.handle()); if (cached != null) { cache.put(proxy.handle(), new Cached(false, cached.freeFunc, cached.boxedType, cached.cleanable)); cached.cleanable.clean(); } } } ``` I can make a pull request if desired.

jwharm commented

2025-05-08 21:23:40 +02:00

(Migrated from github.com)

Thanks for reporting! I would appreciate a PR.