LUCENE-9359: Always call checkFooter in SegmentInfos#readCommit. (#1483)

lucene/CHANGES.txt

@@ -195,6 +195,9 @@ Improvements
 * LUCENE-9342: TotalHits' relation will be EQUAL_TO when the number of hits is lower than TopDocsColector's numHits
   (Tomás Fernández Löbbe)
 
+* LUCENE-9359: SegmentInfos#readCommit now always returns a
+  CorruptIndexException if the content of the file is invalid. (Adrien Grand)
+
 Optimizations
 ---------------------
 

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

@@ -304,7 +304,8 @@ public final class SegmentInfos implements Cloneable, Iterable<SegmentCommitInfo
 
   /** Read the commit from the provided {@link ChecksumIndexInput}. */
   public static final SegmentInfos readCommit(Directory directory, ChecksumIndexInput input, long generation) throws IOException {
-
+    Throwable priorE = null;
+    try {
       // NOTE: as long as we want to throw indexformattooold (vs corruptindexexception), we need
       // to read the magic ourselves.
       int magic = input.readInt();
@@ -426,14 +427,18 @@ public final class SegmentInfos implements Cloneable, Iterable<SegmentCommitInfo
 
       infos.userData = input.readMapOfStrings();
 
-    CodecUtil.checkFooter(input);
-
       // LUCENE-6299: check we are in bounds
       if (totalDocs > IndexWriter.getActualMaxDocs()) {
         throw new CorruptIndexException("Too many documents: an index cannot exceed " + IndexWriter.getActualMaxDocs() + " but readers have total maxDoc=" + totalDocs, input);
       }
 
       return infos;
+    } catch (Throwable t) {
+      priorE = t;
+    } finally {
+      CodecUtil.checkFooter(input, priorE);
+    }
+    throw new Error("Unreachable code");
   }
 
   private static Codec readCodec(DataInput input) throws IOException {

lucene/core/src/test/org/apache/lucene/index/TestSegmentInfos.java

@@ -18,12 +18,16 @@ package org.apache.lucene.index;
 
 
 import org.apache.lucene.codecs.Codec;
+import org.apache.lucene.codecs.CodecUtil;
 import org.apache.lucene.search.Sort;
 import org.apache.lucene.store.BaseDirectoryWrapper;
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.store.IOContext;
+import org.apache.lucene.store.IndexInput;
+import org.apache.lucene.store.IndexOutput;
 import org.apache.lucene.util.LuceneTestCase;
 import org.apache.lucene.util.StringHelper;
+import org.apache.lucene.util.TestUtil;
 import org.apache.lucene.util.Version;
 
 import java.io.IOException;
@@ -178,5 +182,62 @@ public class TestSegmentInfos extends LuceneTestCase {
       assertEquals("clone changed but shouldn't", StringHelper.idToString(id), StringHelper.idToString(clone.getId()));
     }
   }
+
+  public void testBitFlippedTriggersCorruptIndexException() throws IOException {
+    BaseDirectoryWrapper dir = newDirectory();
+    dir.setCheckIndexOnClose(false);
+    byte id[] = StringHelper.randomId();
+    Codec codec = Codec.getDefault();
+
+    SegmentInfos sis = new SegmentInfos(Version.LATEST.major);
+    SegmentInfo info = new SegmentInfo(dir, Version.LATEST, Version.LATEST, "_0", 1, false, Codec.getDefault(),
+                                       Collections.<String,String>emptyMap(), id, Collections.<String,String>emptyMap(), null);
+    info.setFiles(Collections.<String>emptySet());
+    codec.segmentInfoFormat().write(dir, info, IOContext.DEFAULT);
+    SegmentCommitInfo commitInfo = new SegmentCommitInfo(info, 0, 0, -1, -1, -1, StringHelper.randomId());
+    sis.add(commitInfo);
+
+    info = new SegmentInfo(dir, Version.LATEST, Version.LATEST, "_1", 1, false, Codec.getDefault(),
+                           Collections.<String,String>emptyMap(), id, Collections.<String,String>emptyMap(), null);
+    info.setFiles(Collections.<String>emptySet());
+    codec.segmentInfoFormat().write(dir, info, IOContext.DEFAULT);
+    commitInfo = new SegmentCommitInfo(info, 0, 0,-1, -1, -1, StringHelper.randomId());
+    sis.add(commitInfo);
+
+    sis.commit(dir);
+
+    BaseDirectoryWrapper corruptDir = newDirectory();
+    corruptDir.setCheckIndexOnClose(false);
+    boolean corrupt = false;
+    for (String file : dir.listAll()) {
+      if (file.startsWith(IndexFileNames.SEGMENTS)) {
+        try (IndexInput in = dir.openInput(file, IOContext.DEFAULT);
+            IndexOutput out = corruptDir.createOutput(file, IOContext.DEFAULT)) {
+          final long corruptIndex = TestUtil.nextLong(random(), 0, in.length() - 1);
+          out.copyBytes(in, corruptIndex);
+          final int b = Byte.toUnsignedInt(in.readByte()) + TestUtil.nextInt(random(), 0x01, 0xff);
+          out.writeByte((byte) b);
+          out.copyBytes(in, in.length() - in.getFilePointer());
+        }
+        try (IndexInput in = corruptDir.openInput(file, IOContext.DEFAULT)) {
+          CodecUtil.checksumEntireFile(in);
+          if (VERBOSE) {
+            System.out.println("TEST: Altering the file did not update the checksum, aborting...");
+          }
+          return;
+        } catch (CorruptIndexException e) {
+          // ok
+        }
+        corrupt = true;
+      } else if (slowFileExists(corruptDir, file) == false) { // extraFS
+        corruptDir.copyFrom(dir, file, file, IOContext.DEFAULT);
+      }
+    }
+    assertTrue("No segments file found", corrupt);
+
+    expectThrows(CorruptIndexException.class, () -> SegmentInfos.readLatestCommit(corruptDir));
+    dir.close();
+    corruptDir.close();
+  }
 }