Fix UtilCheckerFuzzer: Security exception Update I18nFormatUtil.java#7031
Fix UtilCheckerFuzzer: Security exception Update I18nFormatUtil.java#7031Shivam7-1 wants to merge 7 commits intotypetools:masterfrom
Conversation
|
Hii @mernst Could you please review this PR |
|
@Shivam7-1 your code does not compile. Please at least ensure the code builds locally and tests pass before requesting a review. |
|
@Shivam7-1 you can run |
|
The code still doesn't compile. Also, please do not force-push to GitHub. Force-pushing has no benefit, since we |
There was a problem hiding this comment.
Pull request overview
This PR addresses an OSS-Fuzz issue by making I18nFormatUtil.formatParameterCategories more defensive against malformed i18n MessageFormat patterns, aiming to prevent crashes and resource abuse from hostile inputs.
Changes:
- Wraps
MessageFormatParser.parsein error handling intended to normalize parser failures into a single invalid-format exception. - Adds validation for argument indices (including an upper bound) to avoid pathological indices.
- Refactors map merging logic to use
getOrDefaultfor clarity.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| I18nConversion[] cs; | ||
| try { | ||
| cs = MessageFormatParser.parse(format); | ||
| } catch (Exception e) { | ||
| // Defensive programming: fail gracefully on parse errors | ||
| throw new IllegalArgumentException("Invalid format string: " + format); | ||
| } |
There was a problem hiding this comment.
java.util.IllegalFormatException is an abstract exception type and cannot be instantiated. This code will not compile as written. Use a concrete exception type (e.g., IllegalArgumentException with the original exception as the cause) or rethrow the parser exception after normalizing it to a single unchecked type used by callers.
| if (index < 0 || index > 1000) { // Arbitrary upper bound to prevent abuse | ||
| throw new IllegalArgumentException( | ||
| "Format string contains illegal argument index: " + index); |
There was a problem hiding this comment.
throw new IllegalFormatException(...) will not compile here either (the type is abstract). If you want to signal an invalid format/index, throw IllegalArgumentException (and consider keeping the exception cause) so callers can handle it consistently.
| // Defensive programming: fail gracefully on parse errors | ||
| throw new IllegalArgumentException("Invalid format string: " + format); | ||
| } |
There was a problem hiding this comment.
The exception message concatenates the full (potentially attacker-controlled) format string. For very large inputs this can significantly increase memory usage and produce overly-large logs; consider truncating the string and/or including only its length (and possibly a small prefix) in the error message.
| if (index < 0 || index > 1000) { // Arbitrary upper bound to prevent abuse | ||
| throw new IllegalArgumentException( |
There was a problem hiding this comment.
The hard-coded upper bound 1000 is a magic number that changes accepted formats and will be hard to adjust later. Consider extracting it into a named constant (e.g., MAX_ARGUMENT_INDEX) with a brief rationale, so the security/DoS intent is explicit and configurable.
| try { | ||
| cs = MessageFormatParser.parse(format); | ||
| } catch (Exception e) { | ||
| // Defensive programming: fail gracefully on parse errors | ||
| throw new IllegalArgumentException("Invalid format string: " + format); | ||
| } | ||
|
|
||
| int maxIndex = -1; | ||
| Map<Integer, I18nConversionCategory> conv = new HashMap<>(cs.length); | ||
|
|
||
| for (I18nConversion c : cs) { | ||
| int index = c.index; | ||
| if (index < 0 || index > 1000) { // Arbitrary upper bound to prevent abuse | ||
| throw new IllegalArgumentException( | ||
| "Format string contains illegal argument index: " + index); |
There was a problem hiding this comment.
New behavior is introduced here (wrapping parser failures and rejecting large argument indices). There are existing JUnit tests for I18nFormatUtil (e.g., checker/src/test/java/.../I18nFormatterUnitTest.java), but none appear to cover these new failure modes; please add tests asserting isFormat(...)/hasFormat(...) behavior for (1) malformed patterns that previously crashed the parser and (2) an out-of-range index (e.g., {1001}).
📝 WalkthroughWalkthroughThe I18nFormatUtil.java file was modified to change thrown exception types from IllegalFormatException to IllegalArgumentException and to declare these methods as throwing IllegalArgumentException. MessageFormatParser.parse(format) is now wrapped in a try/catch that throws IllegalArgumentException on parse errors. Conversion indices are validated to be within 0..1000, rejecting out-of-range indices with IllegalArgumentException. Accumulation logic was refactored to use getOrDefault and I18nConversionCategory.intersect, and final result construction now uses getOrDefault defaulting to UNUSED. 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker-util/src/main/java/org/checkerframework/checker/i18nformatter/util/I18nFormatUtil.java`:
- Line 38: The method signature for tryFormatSatisfiability currently declares
"throws IllegalArgumentException", which is an unchecked exception; remove the
throws clause from the method declaration in
I18nFormatUtil.tryFormatSatisfiability to follow Java conventions and keep
behavior unchanged, and if you want to document the possible
IllegalArgumentException add a `@throws` IllegalArgumentException line to the
method Javadoc instead.
- Around line 66-69: The magic upper bound 1000 used in I18nFormatUtil (the
condition "if (index < 0 || index > 1000)") should be extracted to a named
constant (e.g., MAX_ARGUMENT_INDEX) and documented in the method/class Javadoc;
update the check to use that constant and add a Javadoc sentence on the method
(or class) that explains the reason for and value of the limit (to prevent
resource-exhaustion/fuzzing attacks) so callers are aware of the constraint.
- Around line 54-59: The current catch of Exception in I18nFormatUtil around the
call to MessageFormatParser.parse(format) is too broad and drops the original
exception; replace it by catching the specific parse-related exception(s) thrown
by MessageFormatParser.parse (e.g., a ParseException or the exact checked
exception type it declares) and, if you also need to guard against unchecked
errors, catch RuntimeException separately; when rethrowing use exception
chaining by constructing the IllegalArgumentException with the original
exception as the cause (e.g., new IllegalArgumentException("Invalid format
string: "+format, e)) so the stack trace/root cause is preserved.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
checker-util/src/main/java/org/checkerframework/checker/i18nformatter/util/I18nFormatUtil.java
| @SuppressWarnings("nullness:argument") // It's not documented, but passing null as the | ||
| // argument array is supported. | ||
| public static void tryFormatSatisfiability(String format) throws IllegalFormatException { | ||
| public static void tryFormatSatisfiability(String format) throws IllegalArgumentException { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Declaring throws for unchecked exception is unconventional.
IllegalArgumentException extends RuntimeException, so declaring it in the throws clause has no compile-time effect. While it serves as documentation, it's not idiomatic Java. Consider either removing the throws clause or adding a @throws Javadoc tag instead for documentation purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker-util/src/main/java/org/checkerframework/checker/i18nformatter/util/I18nFormatUtil.java`
at line 38, The method signature for tryFormatSatisfiability currently declares
"throws IllegalArgumentException", which is an unchecked exception; remove the
throws clause from the method declaration in
I18nFormatUtil.tryFormatSatisfiability to follow Java conventions and keep
behavior unchanged, and if you want to document the possible
IllegalArgumentException add a `@throws` IllegalArgumentException line to the
method Javadoc instead.
| try { | ||
| cs = MessageFormatParser.parse(format); | ||
| } catch (Exception e) { | ||
| // Defensive programming: fail gracefully on parse errors | ||
| throw new IllegalArgumentException("Invalid format string: " + format); | ||
| } |
There was a problem hiding this comment.
Catching generic Exception loses important context.
Two issues with this exception handling:
- Catching
Exceptionis too broad—it catches all checked and unchecked exceptions indiscriminately. - The original exception is not chained, losing valuable debugging information (stack trace, root cause).
Proposed fix to preserve exception context
I18nConversion[] cs;
try {
cs = MessageFormatParser.parse(format);
- } catch (Exception e) {
+ } catch (IllegalArgumentException e) {
// Defensive programming: fail gracefully on parse errors
- throw new IllegalArgumentException("Invalid format string: " + format);
+ throw new IllegalArgumentException("Invalid format string: " + format, e);
}If other exception types need handling, consider catching them explicitly rather than using a blanket Exception catch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker-util/src/main/java/org/checkerframework/checker/i18nformatter/util/I18nFormatUtil.java`
around lines 54 - 59, The current catch of Exception in I18nFormatUtil around
the call to MessageFormatParser.parse(format) is too broad and drops the
original exception; replace it by catching the specific parse-related
exception(s) thrown by MessageFormatParser.parse (e.g., a ParseException or the
exact checked exception type it declares) and, if you also need to guard against
unchecked errors, catch RuntimeException separately; when rethrowing use
exception chaining by constructing the IllegalArgumentException with the
original exception as the cause (e.g., new IllegalArgumentException("Invalid
format string: "+format, e)) so the stack trace/root cause is preserved.
| if (index < 0 || index > 1000) { // Arbitrary upper bound to prevent abuse | ||
| throw new IllegalArgumentException( | ||
| "Format string contains illegal argument index: " + index); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the 1000 index limit in the method's Javadoc.
The arbitrary upper bound of 1000 is a reasonable safeguard against resource exhaustion attacks (as discovered by the fuzzer). However, this limit should be documented in the method's Javadoc so callers are aware of the constraint. Additionally, consider extracting 1000 to a named constant for clarity.
Proposed improvement
+ private static final int MAX_FORMAT_INDEX = 1000;
+
/**
* Returns a {`@link` I18nConversionCategory} for every conversion found in the format string.
*
* `@param` format the format string to parse
- * `@throws` IllegalArgumentException if the format is not syntactically valid
+ * `@throws` IllegalArgumentException if the format is not syntactically valid, or if any argument
+ * index exceeds {`@value` `#MAX_FORMAT_INDEX`}
*/And update the check:
- if (index < 0 || index > 1000) { // Arbitrary upper bound to prevent abuse
+ if (index < 0 || index > MAX_FORMAT_INDEX) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker-util/src/main/java/org/checkerframework/checker/i18nformatter/util/I18nFormatUtil.java`
around lines 66 - 69, The magic upper bound 1000 used in I18nFormatUtil (the
condition "if (index < 0 || index > 1000)") should be extracted to a named
constant (e.g., MAX_ARGUMENT_INDEX) and documented in the method/class Javadoc;
update the check to use that constant and add a Javadoc sentence on the method
(or class) that explains the reason for and value of the limit (to prevent
resource-exhaustion/fuzzing attacks) so callers are aware of the constraint.
Issue link: https://issues.oss-fuzz.com/issues/387777040
Improves formatParameterCategories by validating format string indices and handling parsing errors safely. Prevents crashes from malformed input.