From 972912336963337aa8a6b9f523c6c4b90858add4 Mon Sep 17 00:00:00 2001 From: Uwe Schindler Date: Sun, 22 Oct 2023 10:58:44 +0200 Subject: [PATCH] Improve handling of NullPointerException in MMapDirectory's IndexInputs (check the "closed" condition) (#12705) --- lucene/CHANGES.txt | 3 +++ .../org/apache/lucene/store/ByteBufferGuard.java | 4 ++++ .../apache/lucene/store/ByteBufferIndexInput.java | 15 +++++++++++---- .../lucene/store/MemorySegmentIndexInput.java | 14 +++++++++++--- .../lucene/store/MemorySegmentIndexInput.java | 14 +++++++++++--- .../lucene/store/MemorySegmentIndexInput.java | 14 +++++++++++--- .../apache/lucene/store/TestMmapDirectory.java | 13 +++++++++++++ 7 files changed, 64 insertions(+), 13 deletions(-) diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index 26536192dd3..8e3fa77ecd0 100644 --- a/lucene/CHANGES.txt +++ b/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) diff --git a/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java b/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java index 43e171b00ff..82f9e6fa997 100644 --- a/lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java +++ b/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: diff --git a/lucene/core/src/java/org/apache/lucene/store/ByteBufferIndexInput.java b/lucene/core/src/java/org/apache/lucene/store/ByteBufferIndexInput.java index 2b3ddc62971..77840df9ed6 100644 --- a/lucene/core/src/java/org/apache/lucene/store/ByteBufferIndexInput.java +++ b/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(RuntimeException unused) { - return new AlreadyClosedException("Already closed: " + this); + AlreadyClosedException alreadyClosed(NullPointerException npe) { + // we use NPE to signal if this input is closed (to not have checks everywhere). If NPE happens, + // 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); } diff --git a/lucene/core/src/java19/org/apache/lucene/store/MemorySegmentIndexInput.java b/lucene/core/src/java19/org/apache/lucene/store/MemorySegmentIndexInput.java index 0d48f5627d7..2c2c65a7b0c 100644 --- a/lucene/core/src/java19/org/apache/lucene/store/MemorySegmentIndexInput.java +++ b/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 unused) { - return new AlreadyClosedException("Already closed: " + this); + AlreadyClosedException alreadyClosed(RuntimeException e) { + // we use NPE to signal if this input is closed (to not have checks everywhere). If NPE happens, + // 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 diff --git a/lucene/core/src/java20/org/apache/lucene/store/MemorySegmentIndexInput.java b/lucene/core/src/java20/org/apache/lucene/store/MemorySegmentIndexInput.java index f175fd68f62..a8685ec2249 100644 --- a/lucene/core/src/java20/org/apache/lucene/store/MemorySegmentIndexInput.java +++ b/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 unused) { - return new AlreadyClosedException("Already closed: " + this); + AlreadyClosedException alreadyClosed(RuntimeException e) { + // we use NPE to signal if this input is closed (to not have checks everywhere). If NPE happens, + // 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 diff --git a/lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInput.java b/lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInput.java index f175fd68f62..a8685ec2249 100644 --- a/lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInput.java +++ b/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 unused) { - return new AlreadyClosedException("Already closed: " + this); + AlreadyClosedException alreadyClosed(RuntimeException e) { + // we use NPE to signal if this input is closed (to not have checks everywhere). If NPE happens, + // 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 diff --git a/lucene/core/src/test/org/apache/lucene/store/TestMmapDirectory.java b/lucene/core/src/test/org/apache/lucene/store/TestMmapDirectory.java index 5597161a3a9..a7156383aab 100644 --- a/lucene/core/src/test/org/apache/lucene/store/TestMmapDirectory.java +++ b/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)); + } + } + } }