LUCENE-5436: RefrenceManager#accquire can result in infinite loop if manager resource is abused outside of the manager

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1565429 13f79535-47bb-0310-9956-ffa450edef68

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

lucene/core/src/java/org/apache/lucene/index/ReaderManager.java

@@ -82,4 +82,9 @@ public final class ReaderManager extends ReferenceManager<DirectoryReader> {
     return reference.tryIncRef();
   }
 
+  @Override
+  protected int getRefCount(DirectoryReader reference) {
+    return reference.getRefCount();
+  }
+
 }

lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java

@@ -92,13 +92,29 @@ public abstract class ReferenceManager<G> 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));
+      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);
+  }
   
   /**
     * <p>
@@ -132,6 +148,11 @@ public abstract class ReferenceManager<G> 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} 

lucene/core/src/java/org/apache/lucene/search/SearcherManager.java

@@ -128,6 +128,11 @@ public final class SearcherManager extends ReferenceManager<IndexSearcher> {
     return reference.getIndexReader().tryIncRef();
   }
 
+  @Override
+  protected int getRefCount(IndexSearcher reference) {
+    return reference.getIndexReader().getRefCount();
+  }
+
   /**
    * Returns <code>true</code> if no changes have occured since this searcher
    * ie. reader was opened, otherwise <code>false</code>.

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();

lucene/facet/src/java/org/apache/lucene/facet/taxonomy/SearcherTaxonomyManager.java

@@ -141,4 +141,9 @@ public class SearcherTaxonomyManager extends ReferenceManager<SearcherTaxonomyMa
       return new SearcherAndTaxonomy(SearcherManager.getSearcher(searcherFactory, newReader), tr);
     }
   }
+
+  @Override
+  protected int getRefCount(SearcherAndTaxonomy reference) {
+    return reference.searcher.getIndexReader().getRefCount();
+  }
 }