From ac63a65d88ef131c3d3b7c6739a575eea749fa7c Mon Sep 17 00:00:00 2001 From: David Mollitor Date: Fri, 11 Sep 2026 13:35:09 +0000 Subject: [PATCH] [MINOR][CORE] Document the JavaUtils.checkArgument format-string contract and fix a concatenating caller `JavaUtils.checkArgument`/`checkState` build the failure message with `String.format(msg, args)`, so `msg` is a format string and callers are meant to pass runtime values as trailing `args` (e.g. `"bad key: %s", key`). The JavaDoc did not say this, and at least one caller concatenated the value into the message instead: JavaUtils.checkArgument(appShuffleInfo != null, "application " + appId + " is not registered or NM was restarted."); Concatenating is unsafe: on the failure path `String.format` treats the already-interpolated text as the format string, so a stray `%` in the value (a percent-encoded token, a Windows path like `%TEMP%`, a `LIKE` pattern, a number such as "50%") makes it throw an `IllegalFormatException` that replaces the intended `IllegalArgumentException` and hides the real reason the check failed. It also builds the message eagerly on every call, including the common success path. This change: - Expands the `checkArgument`/`checkState` JavaDoc to describe the `String.format` contract and warn against interpolating values into `msg`, with the correct `%s` idiom. - Converts the `RemoteBlockPushResolver.validateAndGetAppShuffleInfo` caller to pass `appId` as a format argument. The produced message is unchanged. Co-authored-by: Isaac --- .../shuffle/RemoteBlockPushResolver.java | 2 +- .../apache/spark/network/util/JavaUtils.java | 52 +++++++++++++++++-- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java b/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java index 4a02a0f326368..ceafb6f755641 100644 --- a/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java +++ b/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java @@ -214,7 +214,7 @@ protected AppShuffleInfo validateAndGetAppShuffleInfo(String appId) { // TODO: [SPARK-33236] Change the message when this service is able to handle NM restart AppShuffleInfo appShuffleInfo = appsShuffleInfo.get(appId); JavaUtils.checkArgument(appShuffleInfo != null, - "application " + appId + " is not registered or NM was restarted."); + "application %s is not registered or NM was restarted.", appId); return appShuffleInfo; } diff --git a/common/utils-java/src/main/java/org/apache/spark/network/util/JavaUtils.java b/common/utils-java/src/main/java/org/apache/spark/network/util/JavaUtils.java index 2cf4570488ee0..48c765365a574 100644 --- a/common/utils-java/src/main/java/org/apache/spark/network/util/JavaUtils.java +++ b/common/utils-java/src/main/java/org/apache/spark/network/util/JavaUtils.java @@ -751,8 +751,40 @@ public static boolean isTesting() { .anyMatch(prefix -> osName.regionMatches(true, 0, prefix, 0, prefix.length())); /** - * Throws IllegalArgumentException with the given message if the check is false. - * Keep this clone of CommandBuilderUtils.checkArgument synced with the original. + * Throws an {@link IllegalArgumentException} with a formatted message if {@code check} is + * {@code false}. + * + *

The failure message is produced with {@link String#format(String, Object...)}: {@code msg} + * is the format string and every runtime value must be supplied through {@code args} using a + * conversion such as {@code %s}. Do not concatenate values into {@code msg}. Passing an + * already-interpolated message, for example + * 

{@code checkArgument(cond, "bad key: " + key)}
+ * is a mistake for two reasons: + * + * Instead, pass the values as trailing arguments, as in + * {@code checkArgument(cond, "bad key: %s", key)}, so the message is format-safe (arbitrary + * characters in {@code key} are inserted literally) and is built only when the check fails. + * + *

Keep this clone of {@code CommandBuilderUtils.checkArgument} synced with the original. + * + * @param check the condition that must hold; an exception is thrown when it is {@code false} + * @param msg a {@link String#format(String, Object...)} format string for the failure message; + * do not interpolate runtime values into it, pass them through {@code args} + * @param args the arguments referenced by the format specifiers in {@code msg} + * @throws IllegalArgumentException if {@code check} is {@code false} */ public static void checkArgument(boolean check, String msg, Object... args) { if (!check) { @@ -761,8 +793,20 @@ public static void checkArgument(boolean check, String msg, Object... args) { } /** - * Throws IllegalStateException with the given message if the check is false. - * Keep this clone of CommandBuilderUtils.checkState synced with the original. + * Throws an {@link IllegalStateException} with a formatted message if {@code check} is + * {@code false}. + * + *

{@code msg} is a {@link String#format(String, Object...)} format string; the same + * format-argument contract and caveats described on {@link #checkArgument(boolean, String, + * Object...)} apply here. + * + *

Keep this clone of {@code CommandBuilderUtils.checkState} synced with the original. + * + * @param check the condition that must hold; an exception is thrown when it is {@code false} + * @param msg a {@link String#format(String, Object...)} format string for the failure message; + * do not interpolate runtime values into it, pass them through {@code args} + * @param args the arguments referenced by the format specifiers in {@code msg} + * @throws IllegalStateException if {@code check} is {@code false} */ public static void checkState(boolean check, String msg, Object... args) { if (!check) {