lwjglgamedev / vulkanbook

Online book which introduces the main concepts required to write graphics games or any other applications using Vulkan in Java by using the LWJGL library.
MIT License
270 stars 16 forks source link

VulkanBuffer pointer never cleaned up / JVM Heap corruption #34

Closed tlf30 closed 2 years ago

tlf30 commented 2 years ago

After playing with the VulkanBuffer implementation, I noticed that the pb pointer buffer is never cleaned up. I tried adding pb.free() to the cleanup, but after many VulkanBuffer allocation and deallocations, the JVM heap will become corrupt. To fix this, I replaced PointerBuffer.allocateDirect(1); with MemoryUtil.memAllocPointer(1);/MemoryUtil.memFree(pointerBuffer); inside the vulkan buffer class. The mem free is called on the cleanup of the vulkan buffer. I have no idea or explination as to why the allocate direct causes the jvm heap to become corrupt. But when creating a large number of buffers, then destroying them, the jvm will crash with error 0xC0000374 and not produce any crash log or memory dump. I found the problem by using the LWJGLX debug agent, and it flagged the buffer on the added pb.free() as being non-lwjgl tracked memory.

I originally discovered this in my own application, and checked the code here as I often use this project as a reference. I created a thread with more information on the lwjgl forums: http://forum.lwjgl.org/index.php?topic=7233.msg37410#new

I'm interested to know what you think about why the pb.free() would cause the issue, and am curious if that is why you never included it in the cleanup, or if there is a larger reason that it is not included in the cleanup that I am missing.

I (re)discovered that direct allocated buffers are handled by GC.

Thanks, Trevor

EDIT: I have opened a LWJGL issue: https://github.com/LWJGL/lwjgl3/issues/775

tlf30 commented 2 years ago

I built a simple test with attempting to free a directly allocated buffer, it crashes immediately.

package io.tlf.lwjgl.test;

import org.lwjgl.PointerBuffer;

public class HeapCorruptionTest {
    public static void main(String[] args) {
        while (true) {
            PointerBuffer temp = PointerBuffer.allocateDirect(100);
            temp.free();
        }
    }
}

It looks like the GC will clean it up on its own. But I am not finding anything that directly states that it cannot be freed. But it looks like that is the case. Perhaps I suggest still removing the PointerBuffer.allocateDirect(1); and using the MemoryUtil.memAllocPointer(1); approach with MemoryUtil.memFree(pointerBuffer); as it is more consistent with the rest of the code base.

Spasi commented 2 years ago

Hey @tlf30,

Indeed, buffers allocated via allocateDirect are managed by the GC. Freeing them explicitly will eventually cause a double-free crash when the GC tries to free again.

If you haven't yet, please read Memory management in LWJGL, it should clear things up. The gist of it is:

tlf30 commented 2 years ago

Thank you @Spasi. I'm afraid I re-read your memory management writeup at least once a month (I have it pinned in my favorites), and I feel like I always re-learn something (and probably forget something else 😆).

@lwjglgamedev I still recommend in this instance replacing the direct allocation with a memory util allocation and freeing the memory on the cleanup. Thoughts?

lwjglgamedev commented 2 years ago

Hi, yes, I need to change that. I will update the code as soon as I can. Thanks for reporting, I should have detected that, but I always tend to forget that not using MemoryUtil prevents tracking....

lwjglgamedev commented 2 years ago

Just updated the code. I do like the approach of tightly controlling allocations and deallocations.