Merge pull request #74 from dmitry-timofeev/invalidate-not-owning

Invalidate not-owning handle when it's closed.

Author: diminio1

src/main/java/com/exonum/binding/proxy/AbstractNativeProxy.java

@@ -1,5 +1,7 @@
 package com.exonum.binding.proxy;
 
+import static com.exonum.binding.proxy.StoragePreconditions.checkValid;
+
 /**
  * A proxy of a native object.
  *
@@ -15,22 +17,12 @@ abstract class AbstractNativeProxy implements AutoCloseable {
    */
   static final long INVALID_NATIVE_HANDLE = 0L;
 
-  /**
-   * A native implementation-specific handle.
-   *
-   * <p>This value shall only be passed as an argument to native methods.
-   */
-  final long nativeHandle;
-
   /**
    * Whether this proxy owns the corresponding native object and is responsible to clean it up.
    */
   private final boolean owningHandle;
 
-  /**
-   * Whether the proxy is valid and may be used to access the native object.
-   */
-  private boolean valid;
+  private long nativeHandle;
 
   /**
    * @param nativeHandle a native handle: an implementation-specific reference to a native object.
@@ -40,7 +32,30 @@ abstract class AbstractNativeProxy implements AutoCloseable {
   AbstractNativeProxy(long nativeHandle, boolean owningHandle) {
     this.nativeHandle = nativeHandle;
     this.owningHandle = owningHandle;
-    valid = (nativeHandle != INVALID_NATIVE_HANDLE);
+  }
+
+  /**
+   * Returns a native implementation-specific handle.
+   *
+   * <p>The returned value shall only be passed as an argument to native methods.
+   *
+   * <p>Warning: do not cache the return value, as you won't be able to catch use-after-free.
+   *
+   * @throws IllegalStateException if this native proxy is invalid (closed or nullptr).
+   */
+  final long getNativeHandle() {
+    checkValid(this);
+    return getNativeHandleUnsafe();
+  }
+
+  /**
+   * Returns a native handle.
+   *
+   * <p>If clients ever need to just get a value (e.g., in their {@link #toString()}
+   * implementation), make this package-local.
+   */
+  private long getNativeHandleUnsafe() {
+    return nativeHandle;
   }
 
   /**
@@ -53,24 +68,30 @@ abstract class AbstractNativeProxy implements AutoCloseable {
    */
   @Override
   public final void close() {
-    if (owningHandle && valid) {
+    if (isValid()) {
       try {
-        disposeInternal();
+        if (owningHandle) {
+          disposeInternal();
+        }
       } finally {
-        valid = false;
+        invalidate();
       }
     }
   }
 
+  /** Returns true if the proxy is valid and may be used to access the native object. */
+  final boolean isValid() {
+    return nativeHandle != INVALID_NATIVE_HANDLE;
+  }
+
   /**
    * Destroys the corresponding native object.
    *
-   * <p>This method is only called from {@link #close()} and shall not be called directly.
+   * <p>This method is only called once from {@link #close()} and shall not be called directly.
    */
   abstract void disposeInternal();
 
-  /** Returns true if the proxy is valid and may be used to access the native object. */
-  final boolean isValid() {
-    return valid;
+  private void invalidate() {
+    nativeHandle = INVALID_NATIVE_HANDLE;
   }
 }

src/main/java/com/exonum/binding/proxy/Fork.java

@@ -17,6 +17,6 @@ public Fork(long nativeHandle) {
 
   @Override
   void disposeInternal() {
-    Views.nativeFree(nativeHandle);
+    Views.nativeFree(getNativeHandle());
   }
 }

src/main/java/com/exonum/binding/proxy/LevelDb.java

@@ -8,17 +8,17 @@ public LevelDb(String pathToDb) {
 
   @Override
   public Snapshot createSnapshot() {
-    return new Snapshot(nativeCreateSnapshot(this.nativeHandle));
+    return new Snapshot(nativeCreateSnapshot(getNativeHandle()));
   }
 
   @Override
   public Fork createFork() {
-    return new Fork(nativeCreateFork(this.nativeHandle));
+    return new Fork(nativeCreateFork(getNativeHandle()));
   }
 
   @Override
   void disposeInternal() {
-    nativeFree(nativeHandle);
+    nativeFree(getNativeHandle());
   }
 
   private static native long nativeCreate(String path);

src/main/java/com/exonum/binding/proxy/MapIndexProxy.java

@@ -24,25 +24,25 @@ public MapIndexProxy(View view, byte[] prefix) {
   }
 
   public void put(byte[] key, byte[] value) {
-    nativePut(checkStorageKey(key), checkStorageValue(value), nativeHandle);
+    nativePut(checkStorageKey(key), checkStorageValue(value), getNativeHandle());
   }
 
   public byte[] get(byte[] key) {
-    return nativeGet(checkStorageKey(key), nativeHandle);
+    return nativeGet(checkStorageKey(key), getNativeHandle());
   }
 
   public void remove(byte[] key) {
-    nativeRemove(checkStorageKey(key), nativeHandle);
+    nativeRemove(checkStorageKey(key), getNativeHandle());
   }
 
   public void clear() {
-    nativeClear(nativeHandle);
+    nativeClear(getNativeHandle());
   }
 
   @Override
   void disposeInternal() {
     checkValid(dbView);
-    nativeFree(nativeHandle);
+    nativeFree(getNativeHandle());
   }
 
   private static native long nativeCreate(long viewNativeHandle, byte[] prefix);

src/main/java/com/exonum/binding/proxy/MemoryDb.java

@@ -8,17 +8,17 @@ public MemoryDb() {
 
   @Override
   public Snapshot createSnapshot() {
-    return new Snapshot(nativeCreateSnapshot(nativeHandle));
+    return new Snapshot(nativeCreateSnapshot(getNativeHandle()));
   }
 
   @Override
   public Fork createFork() {
-    return new Fork(nativeCreateFork(nativeHandle));
+    return new Fork(nativeCreateFork(getNativeHandle()));
   }
 
   @Override
   void disposeInternal() {
-    nativeFree(nativeHandle);
+    nativeFree(getNativeHandle());
   }
 
   private static native long nativeCreate();

src/main/java/com/exonum/binding/proxy/Snapshot.java

@@ -29,6 +29,6 @@ public Snapshot(long nativeHandle) {
 
   @Override
   void disposeInternal() {
-    Views.nativeFree(nativeHandle);
+    Views.nativeFree(getNativeHandle());
   }
 }

src/main/java/com/exonum/binding/proxy/View.java

@@ -29,8 +29,4 @@ public abstract class View extends AbstractNativeProxy {
   View(long nativeHandle, boolean owningHandle) {
     super(nativeHandle, owningHandle);
   }
-
-  long getNativeHandle() {
-    return nativeHandle;
-  }
 }

src/test/java/com/exonum/binding/proxy/AbstractNativeProxyTest.java

@@ -55,12 +55,47 @@ public void shallNotBeValidOnceClosed() throws Exception {
     assertFalse(proxy.isValid());
   }
 
+  @Test
+  public void notOwningShallNotBeValidOnceClosed() throws Exception {
+    proxy = new NativeProxyFake(1L, false);
+    proxy.close();
+    assertFalse(proxy.isValid());
+  }
+
   @Test
   public void shallNotBeValidIfInvalidHandle() throws Exception {
     proxy = new NativeProxyFake(INVALID_NATIVE_HANDLE, true);
     assertFalse(proxy.isValid());
   }
 
+  @Test
+  public void getNativeHandle() throws Exception {
+    long expectedNativeHandle = 0x1FL;
+
+    proxy = new NativeProxyFake(expectedNativeHandle, true);
+
+    assertThat(proxy.getNativeHandle(), equalTo(expectedNativeHandle));
+  }
+
+  @Test(expected = IllegalStateException.class)
+  public void getNativeHandleShallFailIfProxyIsClosed() throws Exception {
+    long nativeHandle = 0x1FL;
+
+    proxy = new NativeProxyFake(nativeHandle, true);
+    proxy.close();
+
+    proxy.getNativeHandle();  // boom
+  }
+
+  @Test(expected = IllegalStateException.class)
+  public void getNativeHandleShallFailIfInvalid() throws Exception {
+    long invalidHandle = 0x0L;
+
+    proxy = new NativeProxyFake(invalidHandle, true);
+
+    proxy.getNativeHandle();  // boom
+  }
+
   private static class NativeProxyFake extends AbstractNativeProxy {
 
     int timesDisposed;