Improve handling of NullPointerException in MMapDirectory's IndexInputs (check the "closed" condition) (#12705)

lucene/CHANGES.txt

@@ -183,6 +183,9 @@ Improvements
 
 * GITHUB#12586: Remove over-counting of deleted terms. (Guo Feng)
 
+* GITHUB#12705: Improve handling of NullPointerException in MMapDirectory's IndexInputs.
+  (Uwe Schindler, Michael Sokolov)
+
 Optimizations
 ---------------------
 * GITHUB#12183: Make TermStates#build concurrent. (Shubham Chaudhary)

lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java

@@ -78,6 +78,10 @@ final class ByteBufferGuard {
     }
   }
 
+  public boolean isInvalidated() {
+    return invalidated;
+  }
+
   private void ensureValid() {
     if (invalidated) {
       // this triggers an AlreadyClosedException in ByteBufferIndexInput:

lucene/core/src/java/org/apache/lucene/store/ByteBufferIndexInput.java

@@ -99,9 +99,16 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
     }
   }
 
-  // the unused parameter is just to silence javac about unused variables
+  AlreadyClosedException alreadyClosed(NullPointerException npe) {
-  AlreadyClosedException alreadyClosed(RuntimeException unused) {
+    // we use NPE to signal if this input is closed (to not have checks everywhere). If NPE happens,
-    return new AlreadyClosedException("Already closed: " + this);
+    // we check the "is closed" condition explicitly by checking that our "buffers" are null or
+    // the guard was invalidated.
+    if (this.buffers == null || this.curBuf == null || guard.isInvalidated()) {
+      return new AlreadyClosedException("Already closed: " + this);
+    }
+    // otherwise rethrow unmodified NPE (as it possibly a bug with passing a null parameter to the
+    // IndexInput method):
+    throw npe;
   }
 
   @Override
@@ -459,7 +466,7 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
 
   /** Builds the actual sliced IndexInput (may apply extra offset in subclasses). * */
   protected ByteBufferIndexInput buildSlice(String sliceDescription, long offset, long length) {
-    if (buffers == null) {
+    if (buffers == null || guard.isInvalidated()) {
       throw alreadyClosed(null);
     }
 

lucene/core/src/java19/org/apache/lucene/store/MemorySegmentIndexInput.java

@@ -102,9 +102,17 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
     }
   }
 
-  // the unused parameter is just to silence javac about unused variables
+  AlreadyClosedException alreadyClosed(RuntimeException e) {
-  AlreadyClosedException alreadyClosed(RuntimeException unused) {
+    // we use NPE to signal if this input is closed (to not have checks everywhere). If NPE happens,
-    return new AlreadyClosedException("Already closed: " + this);
+    // we check the "is closed" condition explicitly by checking that our "curSegment" is null.
+    // if it is an IllegalStateException, it can only come from MemorySegment API (other thread has
+    // closed input)
+    if (this.curSegment == null || e instanceof IllegalStateException) {
+      return new AlreadyClosedException("Already closed: " + this);
+    }
+    // otherwise rethrow unmodified NPE/ISE (as it possibly a bug with passing a null parameter to
+    // the IndexInput method):
+    throw e;
   }
 
   @Override

lucene/core/src/java20/org/apache/lucene/store/MemorySegmentIndexInput.java

@@ -100,9 +100,17 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
     }
   }
 
-  // the unused parameter is just to silence javac about unused variables
+  AlreadyClosedException alreadyClosed(RuntimeException e) {
-  AlreadyClosedException alreadyClosed(RuntimeException unused) {
+    // we use NPE to signal if this input is closed (to not have checks everywhere). If NPE happens,
-    return new AlreadyClosedException("Already closed: " + this);
+    // we check the "is closed" condition explicitly by checking that our "curSegment" is null.
+    // if it is an IllegalStateException, it can only come from MemorySegment API (other thread has
+    // closed input)
+    if (this.curSegment == null || e instanceof IllegalStateException) {
+      return new AlreadyClosedException("Already closed: " + this);
+    }
+    // otherwise rethrow unmodified NPE/ISE (as it possibly a bug with passing a null parameter to
+    // the IndexInput method):
+    throw e;
   }
 
   @Override

lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInput.java

@@ -100,9 +100,17 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
     }
   }
 
-  // the unused parameter is just to silence javac about unused variables
+  AlreadyClosedException alreadyClosed(RuntimeException e) {
-  AlreadyClosedException alreadyClosed(RuntimeException unused) {
+    // we use NPE to signal if this input is closed (to not have checks everywhere). If NPE happens,
-    return new AlreadyClosedException("Already closed: " + this);
+    // we check the "is closed" condition explicitly by checking that our "curSegment" is null.
+    // if it is an IllegalStateException, it can only come from MemorySegment API (other thread has
+    // closed input)
+    if (this.curSegment == null || e instanceof IllegalStateException) {
+      return new AlreadyClosedException("Already closed: " + this);
+    }
+    // otherwise rethrow unmodified NPE/ISE (as it possibly a bug with passing a null parameter to
+    // the IndexInput method):
+    throw e;
   }
 
   @Override

lucene/core/src/test/org/apache/lucene/store/TestMmapDirectory.java

@@ -104,4 +104,17 @@ public class TestMmapDirectory extends BaseDirectoryTestCase {
       dir.close();
     }
   }
+
+  public void testNullParamsIndexInput() throws Exception {
+    try (Directory mmapDir = getDirectory(createTempDir("testNullParamsIndexInput"))) {
+      try (IndexOutput out = mmapDir.createOutput("bytes", newIOContext(random()))) {
+        out.alignFilePointer(16);
+      }
+      try (IndexInput in = mmapDir.openInput("bytes", IOContext.DEFAULT)) {
+        assertThrows(NullPointerException.class, () -> in.readBytes(null, 0, 1));
+        assertThrows(NullPointerException.class, () -> in.readFloats(null, 0, 1));
+        assertThrows(NullPointerException.class, () -> in.readLongs(null, 0, 1));
+      }
+    }
+  }
 }