From 1fa91537a9485ea08a0ff834d4306b414150d500 Mon Sep 17 00:00:00 2001 From: Mike McCandless Date: Tue, 9 Aug 2016 05:03:29 -0400 Subject: [PATCH] LUCENE-7409: improve MockDirectoryWrapper's IndexInput to detect if a clone is being used after its parent was closed --- .../lucene/store/MockDirectoryWrapper.java | 2 +- .../lucene/store/MockIndexInputWrapper.java | 27 +++++++++----- .../SlowClosingMockIndexInputWrapper.java | 2 +- .../SlowOpeningMockIndexInputWrapper.java | 2 +- .../store/TestMockDirectoryWrapper.java | 36 +++++++++++++++++++ 5 files changed, 57 insertions(+), 12 deletions(-) diff --git a/lucene/test-framework/src/java/org/apache/lucene/store/MockDirectoryWrapper.java b/lucene/test-framework/src/java/org/apache/lucene/store/MockDirectoryWrapper.java index e78968d452a..1ff9470fb97 100644 --- a/lucene/test-framework/src/java/org/apache/lucene/store/MockDirectoryWrapper.java +++ b/lucene/test-framework/src/java/org/apache/lucene/store/MockDirectoryWrapper.java @@ -771,7 +771,7 @@ public class MockDirectoryWrapper extends BaseDirectoryWrapper { } ii = new SlowOpeningMockIndexInputWrapper(this, name, delegateInput); } else { - ii = new MockIndexInputWrapper(this, name, delegateInput); + ii = new MockIndexInputWrapper(this, name, delegateInput, null); } addFileHandle(ii, name, Handle.Input); return ii; diff --git a/lucene/test-framework/src/java/org/apache/lucene/store/MockIndexInputWrapper.java b/lucene/test-framework/src/java/org/apache/lucene/store/MockIndexInputWrapper.java index f62d67b9392..f68e18cd8ab 100644 --- a/lucene/test-framework/src/java/org/apache/lucene/store/MockIndexInputWrapper.java +++ b/lucene/test-framework/src/java/org/apache/lucene/store/MockIndexInputWrapper.java @@ -30,12 +30,19 @@ public class MockIndexInputWrapper extends IndexInput { private MockDirectoryWrapper dir; final String name; private IndexInput delegate; - private boolean isClone; - private boolean closed; + private volatile boolean closed; - /** Construct an empty output buffer. */ - public MockIndexInputWrapper(MockDirectoryWrapper dir, String name, IndexInput delegate) { + // Which MockIndexInputWrapper we were cloned from, or null if we are not a clone: + private final MockIndexInputWrapper parent; + + /** Sole constructor */ + public MockIndexInputWrapper(MockDirectoryWrapper dir, String name, IndexInput delegate, MockIndexInputWrapper parent) { super("MockIndexInputWrapper(name=" + name + " delegate=" + delegate + ")"); + + // If we are a clone then our parent better not be a clone! + assert parent == null || parent.parent == null; + + this.parent = parent; this.name = name; this.dir = dir; this.delegate = delegate; @@ -54,7 +61,7 @@ public class MockIndexInputWrapper extends IndexInput { // remove the conditional check so we also track that // all clones get closed: assert delegate != null; - if (!isClone) { + if (parent == null) { dir.removeIndexInput(this, name); } dir.maybeThrowDeterministicException(); @@ -62,9 +69,13 @@ public class MockIndexInputWrapper extends IndexInput { } private void ensureOpen() { + // TODO: not great this is a volatile read (closed) ... we should deploy heavy JVM voodoo like SwitchPoint to avoid this if (closed) { throw new RuntimeException("Abusing closed IndexInput!"); } + if (parent != null && parent.closed) { + throw new RuntimeException("Abusing clone of a closed IndexInput!"); + } } @Override @@ -75,8 +86,7 @@ public class MockIndexInputWrapper extends IndexInput { } dir.inputCloneCount.incrementAndGet(); IndexInput iiclone = delegate.clone(); - MockIndexInputWrapper clone = new MockIndexInputWrapper(dir, name, iiclone); - clone.isClone = true; + MockIndexInputWrapper clone = new MockIndexInputWrapper(dir, name, iiclone, parent != null ? parent : this); // Pending resolution on LUCENE-686 we may want to // uncomment this code so that we also track that all // clones get closed: @@ -102,8 +112,7 @@ public class MockIndexInputWrapper extends IndexInput { } dir.inputCloneCount.incrementAndGet(); IndexInput slice = delegate.slice(sliceDescription, offset, length); - MockIndexInputWrapper clone = new MockIndexInputWrapper(dir, sliceDescription, slice); - clone.isClone = true; + MockIndexInputWrapper clone = new MockIndexInputWrapper(dir, sliceDescription, slice, parent != null ? parent : this); return clone; } diff --git a/lucene/test-framework/src/java/org/apache/lucene/store/SlowClosingMockIndexInputWrapper.java b/lucene/test-framework/src/java/org/apache/lucene/store/SlowClosingMockIndexInputWrapper.java index 2be2e2792b3..e6c3857164e 100644 --- a/lucene/test-framework/src/java/org/apache/lucene/store/SlowClosingMockIndexInputWrapper.java +++ b/lucene/test-framework/src/java/org/apache/lucene/store/SlowClosingMockIndexInputWrapper.java @@ -30,7 +30,7 @@ class SlowClosingMockIndexInputWrapper extends MockIndexInputWrapper { public SlowClosingMockIndexInputWrapper(MockDirectoryWrapper dir, String name, IndexInput delegate) { - super(dir, name, delegate); + super(dir, name, delegate, null); } @Override diff --git a/lucene/test-framework/src/java/org/apache/lucene/store/SlowOpeningMockIndexInputWrapper.java b/lucene/test-framework/src/java/org/apache/lucene/store/SlowOpeningMockIndexInputWrapper.java index 4cc2b19665c..1e95451ec3d 100644 --- a/lucene/test-framework/src/java/org/apache/lucene/store/SlowOpeningMockIndexInputWrapper.java +++ b/lucene/test-framework/src/java/org/apache/lucene/store/SlowOpeningMockIndexInputWrapper.java @@ -28,7 +28,7 @@ class SlowOpeningMockIndexInputWrapper extends MockIndexInputWrapper { public SlowOpeningMockIndexInputWrapper(MockDirectoryWrapper dir, String name, IndexInput delegate) throws IOException { - super(dir, name, delegate); + super(dir, name, delegate, null); try { Thread.sleep(50); } catch (InterruptedException ie) { diff --git a/lucene/test-framework/src/test/org/apache/lucene/store/TestMockDirectoryWrapper.java b/lucene/test-framework/src/test/org/apache/lucene/store/TestMockDirectoryWrapper.java index ae453e5155e..145b40b2e5e 100644 --- a/lucene/test-framework/src/test/org/apache/lucene/store/TestMockDirectoryWrapper.java +++ b/lucene/test-framework/src/test/org/apache/lucene/store/TestMockDirectoryWrapper.java @@ -171,4 +171,40 @@ public class TestMockDirectoryWrapper extends BaseDirectoryTestCase { assertTrue("MockDirectoryWrapper on dir=" + dir + " failed to corrupt an unsync'd file", changed); } + + public void testAbuseClosedIndexInput() throws Exception { + MockDirectoryWrapper dir = newMockDirectory(); + IndexOutput out = dir.createOutput("foo", IOContext.DEFAULT); + out.writeByte((byte) 42); + out.close(); + final IndexInput in = dir.openInput("foo", IOContext.DEFAULT); + in.close(); + expectThrows(RuntimeException.class, () -> {in.readByte();}); + dir.close(); + } + + public void testAbuseCloneAfterParentClosed() throws Exception { + MockDirectoryWrapper dir = newMockDirectory(); + IndexOutput out = dir.createOutput("foo", IOContext.DEFAULT); + out.writeByte((byte) 42); + out.close(); + IndexInput in = dir.openInput("foo", IOContext.DEFAULT); + final IndexInput clone = in.clone(); + in.close(); + expectThrows(RuntimeException.class, () -> {clone.readByte();}); + dir.close(); + } + + public void testAbuseCloneOfCloneAfterParentClosed() throws Exception { + MockDirectoryWrapper dir = newMockDirectory(); + IndexOutput out = dir.createOutput("foo", IOContext.DEFAULT); + out.writeByte((byte) 42); + out.close(); + IndexInput in = dir.openInput("foo", IOContext.DEFAULT); + IndexInput clone1 = in.clone(); + IndexInput clone2 = clone1.clone(); + in.close(); + expectThrows(RuntimeException.class, () -> {clone2.readByte();}); + dir.close(); + } }