From edd43858a3fb4e34ca8eed95994ac2f4a737d04f Mon Sep 17 00:00:00 2001
From: yangjie01 <yangjie01@baidu.com>
Date: Wed, 17 Jun 2026 17:26:52 +0800
Subject: [PATCH] [SPARK-57504][CORE] Avoid a potential NullPointerException in
 IndexShuffleBlockResolver.getChecksums when the checksum file cannot be
 opened

### What changes were proposed in this pull request?

`IndexShuffleBlockResolver.getChecksums` opens the checksum file with
`var in: DataInputStream = null` inside a `try` and closes it in
`finally { in.close() }`. The method already catches `IOException`/`EOFException`
to return `null`, so it is meant to degrade gracefully when the file cannot be
read. But the `finally` is not null-safe: if the failure originates in the stream
constructor (before `in` is assigned), it dereferences `null` and throws a
`NullPointerException` that masks the original error.

The two sibling methods in the same file already handle constructor failure
correctly: `checkIndexAndDataFile` constructs the stream in its own `try/catch`,
and `getMergedBlockData` uses `Utils.tryWithResource`. This change makes
`getChecksums` consistent by using `Utils.tryWithResource`, which evaluates the
constructor before its own `try`, so a constructor `IOException` reaches the
existing `catch` and the method returns `null` as intended.

### Why are the changes needed?

`getChecksums` has a latent `NullPointerException`: the non-null-safe
`finally { in.close() }` would mask the real error if opening the checksum file
fails at construction time, which is inconsistent with the rest of the file. No
common code path is known to trigger it today; this is a robustness and
consistency improvement that removes the latent NPE and aligns the method with
its siblings.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Added a unit test in `IndexShuffleBlockResolverSuite` that forces the stream
constructor to fail (a checksum file whose `exists()` returns true but which
cannot be opened) and asserts `getChecksums` returns `null` instead of throwing.
The test fails on the old code (NPE) and passes with the fix.
---
 .../shuffle/IndexShuffleBlockResolver.scala   | 19 +++++++++----------
 .../sort/IndexShuffleBlockResolverSuite.scala | 16 ++++++++++++++++
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala b/core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala
index a46c23447f84..39a7aa4e7ab1 100644
--- a/core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala
+++ b/core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala
@@ -576,22 +576,21 @@ private[spark] class IndexShuffleBlockResolver(
 
   private[shuffle] def getChecksums(checksumFile: File, blockNum: Int): Array[Long] = {
     if (!checksumFile.exists()) return null
-    val checksums = new ArrayBuffer[Long]
     // Read the checksums of blocks
-    var in: DataInputStream = null
     try {
-      in = new DataInputStream(new NioBufferedFileInputStream(checksumFile))
-      while (checksums.size < blockNum) {
-        checksums += in.readLong()
+      Utils.tryWithResource {
+        new DataInputStream(new NioBufferedFileInputStream(checksumFile))
+      } { in =>
+        val checksums = new ArrayBuffer[Long]
+        while (checksums.size < blockNum) {
+          checksums += in.readLong()
+        }
+        checksums.toArray
       }
     } catch {
       case _: IOException | _: EOFException =>
-        return null
-    } finally {
-      in.close()
+        null
     }
-
-    checksums.toArray
   }
 
   /**
diff --git a/core/src/test/scala/org/apache/spark/shuffle/sort/IndexShuffleBlockResolverSuite.scala b/core/src/test/scala/org/apache/spark/shuffle/sort/IndexShuffleBlockResolverSuite.scala
index d374b54c8cb9..32eca21e96d7 100644
--- a/core/src/test/scala/org/apache/spark/shuffle/sort/IndexShuffleBlockResolverSuite.scala
+++ b/core/src/test/scala/org/apache/spark/shuffle/sort/IndexShuffleBlockResolverSuite.scala
@@ -278,6 +278,22 @@ class IndexShuffleBlockResolverSuite extends SparkFunSuite {
     val checksumsFromFile = resolver.getChecksums(checksumFile, 10)
     assert(checksumsInMemory === checksumsFromFile)
   }
+
+  test("SPARK-57504: getChecksums returns null instead of throwing NPE when the " +
+    "checksum file cannot be opened") {
+    val resolver = new IndexShuffleBlockResolver(conf, blockManager)
+    // If opening the checksum file fails, getChecksums is meant to return null: it
+    // already catches IOException/EOFException. But the old non-null-safe
+    // `finally { in.close() }` threw NPE when the failure came from the stream
+    // constructor (`in` was still null), masking the original error. The sibling
+    // methods checkIndexAndDataFile and getMergedBlockData already handle this
+    // correctly. Force a constructor failure with a file that appears to exist but
+    // cannot be opened.
+    val unopenableChecksumFile = new File(tempDir, "missing.checksum") {
+      override def exists(): Boolean = true
+    }
+    assert(resolver.getChecksums(unopenableChecksumFile, 10) === null)
+  }
 }
 
 class SslIndexShuffleBlockResolverSuite extends IndexShuffleBlockResolverSuite {