diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index b5f6dd638ae..0f11eccdfef 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -246,6 +246,12 @@ Bug fixes to byte, before calling Similarity.decodeNormValue. (Peng Cheng via Mike McCandless) +* LUCENE-5436: RefrenceManager#accquire can result in infinite loop if + managed resource is abused outside of the RefrenceManager. Decrementing + the reference without a corresponding incRef() call can cause an infinite + loop. ReferenceManager now throws IllegalStateException if currently managed + resources ref count is 0. (Simon Willnauer) + API Changes * LUCENE-5339: The facet module was simplified/reworked to make the diff --git a/lucene/core/src/java/org/apache/lucene/index/ReaderManager.java b/lucene/core/src/java/org/apache/lucene/index/ReaderManager.java index 5a463af0734..89bb7f0f30c 100644 --- a/lucene/core/src/java/org/apache/lucene/index/ReaderManager.java +++ b/lucene/core/src/java/org/apache/lucene/index/ReaderManager.java @@ -82,4 +82,9 @@ public final class ReaderManager extends ReferenceManager { return reference.tryIncRef(); } + @Override + protected int getRefCount(DirectoryReader reference) { + return reference.getRefCount(); + } + } diff --git a/lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java b/lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java index a6d0e94ffdf..41383cbcf08 100644 --- a/lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java +++ b/lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java @@ -92,12 +92,28 @@ public abstract class ReferenceManager implements Closeable { */ public final G acquire() throws IOException { G ref; + do { if ((ref = current) == null) { throw new AlreadyClosedException(REFERENCE_MANAGER_IS_CLOSED_MSG); } - } while (!tryIncRef(ref)); - return ref; + if (tryIncRef(ref)) { + return ref; + } + if (getRefCount(ref) == 0 && current == ref) { + assert ref != null; + /* if we can't increment the reader but we are + still the current reference the RM is in a + illegal states since we can't make any progress + anymore. The reference is closed but the RM still + holds on to it as the actual instance. + This can only happen if somebody outside of the RM + decrements the refcount without a corresponding increment + since the RM assigns the new reference before counting down + the reference. */ + throw new IllegalStateException("The managed reference has already closed - this is likely a bug when the reference count is modified outside of the ReferenceManager"); + } + } while (true); } /** @@ -132,6 +148,11 @@ public abstract class ReferenceManager implements Closeable { } } + /** + * Returns the current reference count of the given reference. + */ + protected abstract int getRefCount(G reference); + /** * Called after close(), so subclass can free any resources. * @throws IOException if the after close operation in a sub-class throws an {@link IOException} diff --git a/lucene/core/src/java/org/apache/lucene/search/SearcherManager.java b/lucene/core/src/java/org/apache/lucene/search/SearcherManager.java index e0d2ce7fdb7..7305e512cea 100644 --- a/lucene/core/src/java/org/apache/lucene/search/SearcherManager.java +++ b/lucene/core/src/java/org/apache/lucene/search/SearcherManager.java @@ -128,6 +128,11 @@ public final class SearcherManager extends ReferenceManager { return reference.getIndexReader().tryIncRef(); } + @Override + protected int getRefCount(IndexSearcher reference) { + return reference.getIndexReader().getRefCount(); + } + /** * Returns true if no changes have occured since this searcher * ie. reader was opened, otherwise false. diff --git a/lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java b/lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java index 57812637be5..ae2274c5be1 100644 --- a/lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java +++ b/lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java @@ -303,6 +303,37 @@ public class TestSearcherManager extends ThreadedIndexingAndSearchingTestCase { dir.close(); } + public void testReferenceDecrementIllegally() throws Exception { + Directory dir = newDirectory(); + IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig( + TEST_VERSION_CURRENT, new MockAnalyzer(random())).setMergeScheduler(new ConcurrentMergeScheduler())); + SearcherManager sm = new SearcherManager(writer, false, new SearcherFactory()); + writer.addDocument(new Document()); + writer.commit(); + sm.maybeRefreshBlocking(); + + IndexSearcher acquire = sm.acquire(); + IndexSearcher acquire2 = sm.acquire(); + sm.release(acquire); + sm.release(acquire2); + + + acquire = sm.acquire(); + acquire.getIndexReader().decRef(); + sm.release(acquire); + try { + sm.acquire(); + fail("acquire should have thrown an IllegalStateException since we modified the refCount outside of the manager"); + } catch (IllegalStateException ex) { + // + } + + // sm.close(); -- already closed + writer.close(); + dir.close(); + } + + public void testEnsureOpen() throws Exception { Directory dir = newDirectory(); new IndexWriter(dir, new IndexWriterConfig(TEST_VERSION_CURRENT, null)).close(); diff --git a/lucene/facet/src/java/org/apache/lucene/facet/taxonomy/SearcherTaxonomyManager.java b/lucene/facet/src/java/org/apache/lucene/facet/taxonomy/SearcherTaxonomyManager.java index 6de430c758c..561c2b19a5b 100644 --- a/lucene/facet/src/java/org/apache/lucene/facet/taxonomy/SearcherTaxonomyManager.java +++ b/lucene/facet/src/java/org/apache/lucene/facet/taxonomy/SearcherTaxonomyManager.java @@ -141,4 +141,9 @@ public class SearcherTaxonomyManager extends ReferenceManager