svn commit: r1205430 - in /lucene/dev/trunk/lucene: CHANGES.txt src/java/org/apache/lucene/store/MMapDirectory.java src/test/org/apache/lucene/store/TestMultiMMap.java

[us...@apache.org]

Author: uschindler
Date: Wed Nov 23 15:13:50 2011
New Revision: 1205430

URL: http://svn.apache.org/viewvc?rev=1205430&view=rev
Log:
* 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.

Modified:
    lucene/dev/trunk/lucene/CHANGES.txt
    lucene/dev/trunk/lucene/src/java/org/apache/lucene/store/MMapDirectory.java
    lucene/dev/trunk/lucene/src/test/org/apache/lucene/store/TestMultiMMap.java

Modified: lucene/dev/trunk/lucene/CHANGES.txt
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/CHANGES.txt?rev=1205430&r1=1205429&r2=1205430&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/CHANGES.txt (original)
+++ lucene/dev/trunk/lucene/CHANGES.txt Wed Nov 23 15:13:50 2011
@@ -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 =======================
 

Modified: lucene/dev/trunk/lucene/src/java/org/apache/lucene/store/MMapDirectory.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/src/java/org/apache/lucene/store/MMapDirectory.java?rev=1205430&r1=1205429&r2=1205430&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/src/java/org/apache/lucene/store/MMapDirectory.java (original)
+++ lucene/dev/trunk/lucene/src/java/org/apache/lucene/store/MMapDirectory.java Wed Nov 23 15:13:50 2011
@@ -26,6 +26,9 @@ import java.nio.channels.ClosedChannelEx
 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 FSDir
     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 FSDir
           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 FSDir
           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 FSDir
         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 FSDir
         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 FSDir
         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 FSDir
           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 FSDir
       }
       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 FSDir
       } 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;
+    }
   }
 
 }

Modified: lucene/dev/trunk/lucene/src/test/org/apache/lucene/store/TestMultiMMap.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/src/test/org/apache/lucene/store/TestMultiMMap.java?rev=1205430&r1=1205429&r2=1205430&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/src/test/org/apache/lucene/store/TestMultiMMap.java (original)
+++ lucene/dev/trunk/lucene/src/test/org/apache/lucene/store/TestMultiMMap.java Wed Nov 23 15:13:50 2011
@@ -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 Lucen
     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++) {