[PR] [format] Close the row-file input stream when the footer or index fails to parse [paimon]

["PDGGK (via GitHub)" <gi...@apache.org>]

PDGGK opened a new pull request, #9258:
URL: https://github.com/apache/paimon/pull/9258

   ### Purpose
   
   `RowFormatReaderFactory#createReader` opens the input stream on its first line and hands ownership to `RowFormatReader` on its last. Everything in between parses offsets and lengths taken out of the file itself, and any of it can throw on a truncated or corrupt file:
   
   ```java
   SeekableInputStream in = fileIO.newInputStream(path);
   
   in.seek(tailOffset);
   IOUtils.readFully(in, tailBuf);
   RowFileFooter footer = RowFileFooter.readFrom(tailBuf, tailSize - RowFileFooter.FOOTER_SIZE);
   ...
   byte[] indexData = new byte[footer.indexLength];                     // length from the file
   System.arraycopy(tailBuf, indexOffsetInBuf, indexData, 0, footer.indexLength);
   blockIndex = RowBlockIndex.readFrom(indexData);
   
   return new RowFormatReader(in, ...);                                 // ownership passes here
   ```
   
   Concretely:
   
   * a corrupt or truncated tail — `RowFileFooter.readFrom` throws `IOException("Invalid row file magic: …")`;
   * a zero-length file, which is what an aborted write leaves behind — `tailSize` is 0, so the footer is read at offset `-32` and the array access throws;
   * a bad `indexLength` or `indexOffset` — `new byte[…]` or the `arraycopy` throws.
   
   On every one of those, `in` is a local that was never handed out, so nothing can close it.
   
   ### Why this repeats rather than happening once
   
   `DataFileRecordReader.createReader` wraps this call:
   
   ```java
   try {
       return readerFactory.createReader(context);
   } catch (Exception e) {
       ...
       if (ignoreCorruptException(e, ignoreCorruptFiles)) {
           LOG.warn("Failed to create FileRecordReader for file: {}, ignore exception", ...);
           return null;                       // and the scan moves on
       }
   ```
   
   and `ignoreCorruptException` accepts `IOException`, `RuntimeException` and `InternalError` — which covers all of the failures above. So with `ignore-corrupt-files` enabled, a scan over a directory containing corrupt `row` files leaks one descriptor per file and keeps going, which is exactly the configuration in which nobody notices.
   
   ### What changes
   
   The body is wrapped so the stream is closed if ownership never transfers:
   
   ```java
   try {
       ...
       return new RowFormatReader(in, ...);
   } catch (Throwable t) {
       IOUtils.closeQuietly(in);
       throw t;
   }
   ```
   
   `Throwable` rather than `Exception` because `new byte[footer.indexLength]` with a corrupt length can raise an `Error`, and the caller's ignorable set already includes `InternalError`. `IOUtils` was already imported here for `readFully`.
   
   Not try-with-resources: on the success path the stream must stay open, since `RowFormatReader` owns it from then on and closes it through `BlockPrefetcher`.
   
   ### Blast radius
   
   None on a well-formed file — the only added path is the catch, and the success path returns exactly as before. `RowFormatReadWriteTest` (27) and `BlockPrefetcherTest` (14) pass unchanged.
   
   ### Tests
   
   `RowFormatReaderFactoryLeakTest`, two cases, over a `LocalFileIO` subclass that counts input streams opened and not yet closed: a 128-byte file of zeros (fails the magic check) and a zero-length file (fails in the footer read). Both assert the count is back to zero after the failure.
   
   Removing the `closeQuietly` fails both:
   
   ```
   Tests run: 2, Failures: 2, Errors: 0
     aCorruptFooterDoesNotLeakTheInputStream
     anEmptyFileDoesNotLeakTheInputStream
   ```
   
   `RowFormatReaderFactoryLeakTest`, `RowFormatReadWriteTest` and `BlockPrefetcherTest`: 43 tests, 0 failures. `spotless:apply` and `checkstyle:check` on `paimon-format` are clean.
   
   ### API and Format
   
   No change to any public signature, option or on-disk format.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [format] Close the row-file input stream when the footer or index fails to parse [paimon]

["JingsongLi (via GitHub)" <gi...@apache.org>]

JingsongLi merged PR #9258:
URL: https://github.com/apache/paimon/pull/9258


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [format] Close the row-file input stream when the footer or index fails to parse [paimon]

["JingsongLi (via GitHub)" <gi...@apache.org>]

JingsongLi commented on PR #9258:
URL: https://github.com/apache/paimon/pull/9258#issuecomment-5350420159

   +1


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org