Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Callback.free() has a race condition with DebugAllocator enabled #444

Closed
Dinnerbone opened this issue Feb 5, 2019 · 1 comment
Closed

Comments

@Dinnerbone
Copy link

Environment

  • LWJGL version: 3.2.1
  • LWJGL build #: 12
  • Java version: 1.8.0_171
  • Platform: Windows, Linux & macOS
  • Module: core

Description

When running with the debug allocator (-Dorg.lwjgl.util.DebugAllocator=true), Callback.free(functionPointer) may throw an exception if more than one thread is using MemoryManage at the same time. This is because we free up the memory in the real allocator before informing the debugger that it's free - and if something allocates memory inbetween these two statements on another thread, the debugger may consider this an invalid reallocation. Relevant lines: https://github.com/LWJGL/lwjgl3/blob/3.2.1/modules/lwjgl/core/src/main/java/org/lwjgl/system/Callback.java#L187-L194

I've noticed that elsewhere in MemoryTracker we do the opposite order of events: inform debugger that it's free, and then actually free it up. This avoids the problem, but technically is still vulnerable to a race condition: if something is actually allocated the same memory twice without freeing it, then the DebugAllocator won't notice.

Reproducable example

import org.lwjgl.stb.STBIReadCallback;

public class CallbackRaceCondition {
    private static boolean crashed = false;

    public static void main(final String[] args) {
        for (int i = 0; i < 2; i++) {
            new Thread(CallbackRaceCondition::work).start();
        }
    }

    private static void work() {
        try {
            while (!crashed) {
                STBIReadCallback callback = STBIReadCallback.create(CallbackRaceCondition::read);
                callback.close();
            }
        } finally {
            crashed = true;
        }
    }

    static int read(long user, long data, int size) {
        return 0;
    }
}

This technically requires stb module, but anything that inherits from Callback will have the same problem (but I couldn't find a nice core Callback to demo with).
This code spawns 2 threads that will repeatedly allocate and free a Callback. If you run it with -Dorg.lwjgl.util.DebugAllocator=true, you should encounter the following crash:

Exception in thread "Thread-1" java.lang.IllegalStateException: The memory address specified is already being tracked: 0x1FC20000
	at org.lwjgl.system.MemoryManage$DebugAllocator.track(MemoryManage.java:245)
	at org.lwjgl.system.Callback.create(Callback.java:133)
	at org.lwjgl.system.CallbackI.address(CallbackI.java:34)
	at org.lwjgl.stb.STBIReadCallback.create(STBIReadCallback.java:52)
	at com.mojang.CallbackRaceCondition.work(CallbackRaceCondition.java:17)
	at java.lang.Thread.run(Thread.java:748)
@Spasi Spasi added the Type: Bug label Feb 5, 2019
@Spasi Spasi closed this as completed in 228a140 Feb 7, 2019
@Spasi
Copy link
Member

Spasi commented Feb 7, 2019

Thank you @Dinnerbone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants