* LUCENE-3588: Try harder to prevent SIGSEGV on cloned MMapIndexInputs: Previous versions of Lucene could SIGSEGV the JVM if you try to access the clone of an IndexInput retrieved from MMapDirectory. This security fix prevents this as best as it can by throwing AlreadyClosedException also on clones.

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

lucene/CHANGES.txt

@@ -640,7 +640,13 @@ Bug fixes
   
 ======================= Lucene 3.6.0 =======================
 
-(No Changes)
+Security fixes
+
+* LUCENE-3588: Try harder to prevent SIGSEGV on cloned MMapIndexInputs:
+  Previous versions of Lucene could SIGSEGV the JVM if you try to access
+  the clone of an IndexInput retrieved from MMapDirectory. This security fix
+  prevents this as best as it can by throwing AlreadyClosedException
+  also on clones.  (Uwe Schindler, Robert Muir)
 
 ======================= Lucene 3.5.0 =======================
 

lucene/src/java/org/apache/lucene/store/MMapDirectory.java

@@ -26,6 +26,9 @@ import java.nio.channels.ClosedChannelException; // javadoc @link
 import java.nio.channels.FileChannel;
 import java.nio.channels.FileChannel.MapMode;
 
+import java.util.Map;
+import java.util.WeakHashMap;
+
 import java.security.AccessController;
 import java.security.PrivilegedExceptionAction;
 import java.security.PrivilegedActionException;
@@ -256,6 +259,7 @@ public class MMapDirectory extends FSDirectory {
     private ByteBuffer curBuf; // redundant for speed: buffers[curBufIndex]
   
     private boolean isClone = false;
+    private final Map<MMapIndexInput,Boolean> clones = new WeakHashMap<MMapIndexInput,Boolean>();
 
     MMapIndexInput(String resourceDescription, RandomAccessFile raf, long offset, long length, int chunkSizePower) throws IOException {
       super(resourceDescription);
@@ -304,6 +308,8 @@ public class MMapDirectory extends FSDirectory {
           curBuf.position(0);
         } while (!curBuf.hasRemaining());
         return curBuf.get();
+      } catch (NullPointerException npe) {
+        throw new AlreadyClosedException("MMapIndexInput already closed: " + this);
       }
     }
   
@@ -326,6 +332,8 @@ public class MMapDirectory extends FSDirectory {
           curAvail = curBuf.remaining();
         }
         curBuf.get(b, offset, len);
+      } catch (NullPointerException npe) {
+        throw new AlreadyClosedException("MMapIndexInput already closed: " + this);
       }
     }
   
@@ -335,6 +343,8 @@ public class MMapDirectory extends FSDirectory {
         return curBuf.getShort();
       } catch (BufferUnderflowException e) {
         return super.readShort();
+      } catch (NullPointerException npe) {
+        throw new AlreadyClosedException("MMapIndexInput already closed: " + this);
       }
     }
 
@@ -344,6 +354,8 @@ public class MMapDirectory extends FSDirectory {
         return curBuf.getInt();
       } catch (BufferUnderflowException e) {
         return super.readInt();
+      } catch (NullPointerException npe) {
+        throw new AlreadyClosedException("MMapIndexInput already closed: " + this);
       }
     }
 
@@ -353,6 +365,8 @@ public class MMapDirectory extends FSDirectory {
         return curBuf.getLong();
       } catch (BufferUnderflowException e) {
         return super.readLong();
+      } catch (NullPointerException npe) {
+        throw new AlreadyClosedException("MMapIndexInput already closed: " + this);
       }
     }
     
@@ -381,6 +395,8 @@ public class MMapDirectory extends FSDirectory {
           throw new IllegalArgumentException("Seeking to negative position: " + this);
         }
         throw new IOException("seek past EOF: " + this);
+      } catch (NullPointerException npe) {
+        throw new AlreadyClosedException("MMapIndexInput already closed: " + this);
       }
     }
   
@@ -396,9 +412,9 @@ public class MMapDirectory extends FSDirectory {
       }
       final MMapIndexInput clone = (MMapIndexInput)super.clone();
       clone.isClone = true;
+      // we keep clone.clones, so it shares the same map with original and we have no additional cost on clones
+      assert clone.clones == this.clones;
       clone.buffers = new ByteBuffer[buffers.length];
-      // Since most clones will use only one buffer, duplicate() could also be
-      // done lazy in clones, e.g. when adapting curBuf.
       for (int bufNr = 0; bufNr < buffers.length; bufNr++) {
         clone.buffers[bufNr] = buffers[bufNr].duplicate();
       }
@@ -407,26 +423,55 @@ public class MMapDirectory extends FSDirectory {
       } catch(IOException ioe) {
         throw new RuntimeException("Should never happen: " + this, ioe);
       }
+      
+      // register the new clone in our clone list to clean it up on closing:
+      synchronized(this.clones) {
+        this.clones.put(clone, Boolean.TRUE);
+      }
+      
       return clone;
     }
+    
+    private void unsetBuffers() {
+      buffers = null;
+      curBuf = null;
+      curBufIndex = 0;
+    }
   
     @Override
     public void close() throws IOException {
-      curBuf = null; curBufIndex = 0;
       try {
         if (isClone || buffers == null) return;
-        for (int bufNr = 0; bufNr < buffers.length; bufNr++) {
-          // unmap the buffer (if enabled) and at least unset it for GC
-          try {
-            cleanMapping(buffers[bufNr]);
-          } finally {
-            buffers[bufNr] = null;
+        
+        // for extra safety unset also all clones' buffers:
+        synchronized(this.clones) {
+          for (final MMapIndexInput clone : this.clones.keySet()) {
+            assert clone.isClone;
+            clone.unsetBuffers();
           }
+          this.clones.clear();
+        }
+        
+        curBuf = null; curBufIndex = 0; // nuke curr pointer early
+        for (int bufNr = 0; bufNr < buffers.length; bufNr++) {
+          cleanMapping(buffers[bufNr]);
         }
       } finally {
-        buffers = null;
+        unsetBuffers();
       }
     }
+
+    // make sure we have identity on equals/hashCode for WeakHashMap
+    @Override
+    public int hashCode() {
+      return System.identityHashCode(this);
+    }
+
+    // make sure we have identity on equals/hashCode for WeakHashMap
+    @Override
+    public boolean equals(Object obj) {
+      return obj == this;
+    }
   }
 
 }

lucene/src/test/org/apache/lucene/store/TestMultiMMap.java

@@ -18,6 +18,7 @@ package org.apache.lucene.store;
  */
 
 import java.io.File;
+import java.io.IOException;
 import java.util.Random;
 
 import org.apache.lucene.analysis.MockAnalyzer;
@@ -47,6 +48,35 @@ public class TestMultiMMap extends LuceneTestCase {
     workDir = _TestUtil.getTempDir("TestMultiMMap");
     workDir.mkdirs();
   }
+  
+  public void testCloneSafety() throws Exception {
+    MMapDirectory mmapDir = new MMapDirectory(_TestUtil.getTempDir("testCloneSafety"));
+    IndexOutput io = mmapDir.createOutput("bytes", newIOContext(random));
+    io.writeVInt(5);
+    io.close();
+    IndexInput one = mmapDir.openInput("bytes", IOContext.DEFAULT);
+    IndexInput two = (IndexInput) one.clone();
+    IndexInput three = (IndexInput) two.clone(); // clone of clone
+    one.close();
+    try {
+      one.readVInt();
+      fail("Must throw AlreadyClosedException");
+    } catch (AlreadyClosedException ignore) {
+      // pass
+    }
+    try {
+      two.readVInt();
+      fail("Must throw AlreadyClosedException");
+    } catch (AlreadyClosedException ignore) {
+      // pass
+    }
+    try {
+      three.readVInt();
+      fail("Must throw AlreadyClosedExveption");
+    } catch (AlreadyClosedException ignore) {
+      // pass
+    }
+  }
 
   public void testSeekZero() throws Exception {
     for (int i = 0; i < 31; i++) {