Using Memory#close can throw NPE if cleanable not initialized

[dbwiddis]

1. Version of JNA and related jars
   5.12.0

2. Version and vendor of the java virtual machine
   OpenJDK 64-Bit Server VM Homebrew (build 18.0.1.1+0, mixed mode, sharing)

3. Operating system
   macOS 12.4 (Monterey)

4. System architecture (CPU type, bitness of the JVM)
   64-bit Intel Core i9

5. Complete description of the problem
   In Remove use of finalizers in JNA and improve concurrency for Memory, CallbackReference and NativeLibrary #1402, a close() method was added to Memory, which also implemented Closeable (which extends AutoCloseable on newer JDKs). The implementation assumes the private instance variable cleanable still exists:
   

   jna/src/com/sun/jna/Memory.java

   Lines 182 to 186 in 1eec7dd

   	/** Free the native memory and set peer to zero */
   	public void close() {
   	peer = 0;
   	cleanable.clean();
   	}

However, cleanable is of type CleanerRef which extends PhantomReference and thus calling it in code does not imply reachability. When the Cleaner does its work, the reference is removed from the queue and the instance variable becomes null.

Executing close() in this situation can result in an NPE.

6. Steps to reproduce

I updated my project to use try-with-resources blocks on all of my Memory allocations, and added AutoCloseable wrapper classes for (nearly all of) my uses of classes which extend ByReference and Structure. One such example:

class CloseableLUID extends LUID implements AutoCloseable {
    @Override
    public void close() {
        Util.freeMemory(getPointer());
    }
}

and the Util class:

public static void freeMemory(Pointer p) {
    if (p instanceof Memory) {
        ((Memory) p).close();
    }
}

Then, in a method:

try (CloseableLUID luid = new CloseableLUID()) {
    // ...
} // exception occurs here when try-with-resources calls close()

Hundreds of similar implementations like this worked well, but this one fails.

Possibly/probably related, it's in an initializer of one of the first classes to be instantiated, and may very well have been the very first reference in the cleaner queue, giving it a significant advantage in a race condition.

=> java.lang.ExceptionInInitializerError
   oshi.SystemInfo.createOperatingSystem(SystemInfo.java:104)
   oshi.util.Memoizer$1.get(Memoizer.java:87)
   oshi.SystemInfo.getOperatingSystem(SystemInfo.java:98)
   oshi.SystemInfoTest.main(SystemInfoTest.java:102)
   oshi.SystemInfoTest.testPlatformEnum(SystemInfoTest.java:87)
   [...]
 Caused by: java.lang.NullPointerException
   com.sun.jna.Memory.close(Memory.java:185)
   oshi.util.Util.freeMemory(Util.java:112)
   oshi.jna.Struct$CloseableLUID.close(Struct.java:193)
   oshi.software.os.windows.WindowsOperatingSystem.enableDebugPrivilege(WindowsOperatingSystem.java:485)
   oshi.software.os.windows.WindowsOperatingSystem.<clinit>(WindowsOperatingSystem.java:124)

My tests seem to be running fine by removing this one mapping, so this is likely an exceedingly rare race condition.

[matthiasblaesing]

There can be no race condition as cleanable is never set to null. BUT there is a code path that can be hit, where cleanable is not set:

jna/src/com/sun/jna/Memory.java

Lines 124 to 126 in 1eec7dd

	protected Memory() {
	super();
	}

The code is used by the inner class SharedMemory. So I suggest to make the cleanable field final, set it explicity to null in the protected constructor and check for null in close. I would not use exception handling in close, as that might be a hot path, SharedMemory is not that uncommon and handling exceptions is expensive.

[dbwiddis]

There can be no race condition as cleanable is never set to null. BUT there is a code path that can be hit, where cleanable is not set

While this is true and I generally agree with the approach to mitigate (explicitly set null and null-check) I avoided using close() in situations like that.

But the situation I encountered was a new instantiation of a Structure. The no-arg constructor code path doesn't explain the symptom here, so I want to make sure I understand the cause for the subclass of LUID.

The default call to the no-arg constructor would have triggered allocateMemory(CALCULATE_SIZE) which would eventually assign this.memory = autoAllocate(size) (where size=8). But AutoAllocated(int) calls super(int) which should have (with upcast) used the Memory(long) constructor and set cleanable. So something else is happening.

[matthiasblaesing]

Could you add a logger in #freeMemory and show which subclass causes the NullPointerException?

[dbwiddis]

Well, now I'm even more confused. There's some SharedMemory involved but that's not where the exception is being thrown.

The LUID appears to share the middle 8 bytes of a 16-byte allocation. But the memory address that causes the NPE is nowhere near those 16 bytes. And how is that conditional called twice?!

    class CloseableLUID extends LUID implements AutoCloseable {
        @Override
        public void close() {
            System.out.println("Close: " + this.toString());
            System.out.println("Freeing: " + this.getPointer().toString());
            Util.freeMemory(getPointer());
        }
    }

    public static void freeMemory(Pointer p) {
        if (p instanceof Memory) {
            System.out.println("Free p class: " + p.getClass());
            System.out.println("Free p toString: " + ((Memory) p).toString());
            ((Memory) p).close();
        }
    }

Output:

Close: Struct$CloseableLUID(allocated@0x1531d9dc (8 bytes) (shared from allocated@0x1531d9dc (12 bytes) (shared from auto-allocated@0x1531d9d8 (16 bytes)))) {
  int LowPart@0x0=0x0014
  int HighPart@0x4=0x0000
}
Freeing: allocated@0x1531d9dc (8 bytes) (shared from allocated@0x1531d9dc (12 bytes) (shared from auto-allocated@0x1531d9d8 (16 bytes)))
Free p class: class com.sun.jna.Memory$SharedMemory
Free p toString: allocated@0x1531d9dc (8 bytes) (shared from allocated@0x1531d9dc (12 bytes) (shared from auto-allocated@0x1531d9d8 (16 bytes)))
Free p class: class com.sun.jna.Memory
Free p toString: allocated@0x14e41c68 (4 bytes)
Exception in thread "main" java.lang.ExceptionInInitializerError
	at oshi.SystemInfo.createOperatingSystem(SystemInfo.java:104)
	at oshi.util.Memoizer$1.get(Memoizer.java:87)
	at oshi.SystemInfo.getOperatingSystem(SystemInfo.java:98)
	at oshi.SystemInfoTest.main(SystemInfoTest.java:102)
Caused by: java.lang.NullPointerException
	at com.sun.jna.Memory.close(Memory.java:185)
	at oshi.util.Util.freeMemory(Util.java:114)
	at oshi.jna.Struct$CloseableLUID.close(Struct.java:195)
	at oshi.software.os.windows.WindowsOperatingSystem.enableDebugPrivilege(WindowsOperatingSystem.java:485)
	at oshi.software.os.windows.WindowsOperatingSystem.<clinit>(WindowsOperatingSystem.java:124)
	... 4 more

[dbwiddis]

OK, mystery somewhat solved.

The extra output is actually from the CloseableHANDLEByReference() enclosing this whole bit that is also being closed at the same time:

        try (CloseableHANDLEByReference hToken = new CloseableHANDLEByReference()) {
            // ...
            try (CloseableLUID luid = new CloseableLUID()) {
                // ...
            } finally {
                Kernel32.INSTANCE.CloseHandle(hToken.getValue());
            }
        }

That is somewhat similarly mapped:

    class CloseableHANDLEByReference extends HANDLEByReference implements AutoCloseable {
        @Override
        public void close() {
            Util.freeMemory(getPointer());
        }
    }

And of course the ByReference class instantiates its own Memory as part of that.

But the stack trace is associated with the earlier LUID output which is somehow using SharedMemory. But where is it being shared from?

[dbwiddis]

Ultimately I think you're right and the SharedMemory subclass is the issue. I'm not understanding why I get SharedMemory for the LUID but that's a different topic.

[dbwiddis]

OK, I finally have it all sorted. (I think.)

The original LUID had its own memory but then it was used to create a LUID_AND_ATTRIBUTES which was also used in a TOKEN_PRIVILEGES structure. Since each of these needed more memory than was previously allocated, the original allocation went away and was replaced by the shared allocation.

Mystery solved, and PR ready for review.