fix(#4708): resolve ArrayIndexOutOfBoundsException in debug mode for EoSyntax#4709
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughLogging test dependencies were adjusted in eo-parser's POM, EoSyntax's error-summary log message was simplified, and a new test was added to verify parsing under DEBUG logging. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)eo-parser/src/test/java/org/eolang/parser/EoSyntaxTest.java (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
🔇 Additional comments (3)
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 |
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eo-parser/pom.xml(1 hunks)eo-parser/src/main/java/org/eolang/parser/EoSyntax.java(1 hunks)eo-parser/src/test/java/org/eolang/parser/EoSyntaxTest.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Agent
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: integration
- GitHub Check: qulice
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: ort
- GitHub Check: trufflehog
- GitHub Check: snippets
- GitHub Check: benchmark
- GitHub Check: pr-size
🔇 Additional comments (4)
eo-parser/pom.xml (1)
110-113: LGTM! Test scope is appropriate.Restricting log4j to test scope is the correct approach, as it's only used in test code.
eo-parser/src/test/java/org/eolang/parser/EoSyntaxTest.java (2)
21-21: LGTM! Import is necessary for the test.The import is required for manipulating log levels in the test that verifies debug mode behavior.
75-92: Excellent test coverage for the bug fix!The test effectively verifies that parsing doesn't throw an ArrayIndexOutOfBoundsException when debug logging is enabled. The test properly manages log level state by saving and restoring it.
eo-parser/src/main/java/org/eolang/parser/EoSyntax.java (1)
205-209: Perfect fix for the ArrayIndexOutOfBoundsException!The root cause was a format string mismatch: the original message had three placeholders but only two arguments were provided. Removing the unused
%splaceholder resolves the issue while preserving essential debugging information.
| <dependency> | ||
| <groupId>ch.qos.reload4j</groupId> | ||
| <artifactId>reload4j</artifactId> | ||
| <version>1.2.22</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
curl -s "https://search.maven.org/solrsearch/select?q=g:ch.qos.reload4j+AND+a:reload4j&rows=10&wt=json" | jq -r '.response.docs[] | "\(.v)"' | sort -V | tail -10Repository: objectionary/eo
Length of output: 174
🌐 Web query:
reload4j latest version security advisories
💡 Result:
Summary — latest release and security advisories
- Latest release: reload4j ch.qos.reload4j:reload4j 1.2.26 (released Dec 15, 2024). [1]
- Primary security advisories / known issues (and minimum fixed versions):
- XML External Entity (XXE) injection — affects versions < 1.2.22; upgrade to ≥ 1.2.22 (recommend latest). [2][3]
- Deserialization of untrusted data — CVE-2022-23307; fixed in ≥ 1.2.18.1. [2][4]
- SQL injection (JDBCAppender) — CVE-2022-23305; fixed in ≥ 1.2.18.2. [2][5]
- SMTP appender certificate host‑mismatch (MitM) — CVE-2020-9488; fixed in ≥ 1.2.18.3. [2][6]
- Note: Snyk and other security trackers list these issues and mark 1.2.26 as the current non‑vulnerable release; some downstream vendors (e.g., IBM) have published bulletins referencing XXE impact in their products. Upgrade to 1.2.26 (or the latest patched release) and review your use of appenders (JMS/JDBC/SMTP/XML parsing) for exposure. [2][3][1][7]
Sources
- reload4j project site (latest release notes). [1]
- Snyk reload4j vulnerability summary (lists vulnerabilities and fixed versions). [2]
- Snyk advisory: XXE in reload4j (upgrade to ≥1.2.22). [3]
- Snyk / CVE-2022-23307 (deserialization). [4]
- Snyk / CVE-2022-23305 (SQL injection). [5]
- Snyk / CVE-2020-9488 (SMTP MitM). [6]
- IBM security bulletin referencing reload4j XXE impact. [7]
Would you like step-by-step upgrade guidance or a checklist to scan your codebase for vulnerable usages?
🌐 Web query:
ch.qos.reload4j:reload4j:1.2.22 security vulnerabilities
💡 Result:
Short answer: There are no widely reported direct vulnerabilities affecting ch.qos.reload4j:reload4j version 1.2.22. Version 1.2.22 was released to fix a newly discovered XML External Entity (XXE) issue in Chainsaw; security databases (Snyk) report the XXE as affecting versions before 1.2.22 and recommend upgrading to 1.2.22 or later. [1][2][3]
Sources
- reload4j release notes / project page (notes that 1.2.22 fixes an XXE in Chainsaw). [1]
- Snyk vulnerability entry for reload4j (XXE reported affecting versions < 1.2.22; recommends 1.2.22+). [2]
- Maven / release metadata for 1.2.22. [3]
Update reload4j to version 1.2.26 (latest release).
Version 1.2.22 is secure and addresses known XXE vulnerabilities, but version 1.2.26 is now current (released Dec 15, 2024). Update the dependency version to 1.2.26 to align with the latest security patches and improvements.
🤖 Prompt for AI Agents
In eo-parser/pom.xml around lines 114 to 119 the reload4j test dependency is
pinned to version 1.2.22; update the <version> value to 1.2.26 to use the latest
release. Edit the dependency block and change the version element from 1.2.22 to
1.2.26, then run a quick mvn -q test to ensure no test-related regressions.
There was a problem hiding this comment.
Pull request overview
This PR fixes an ArrayIndexOutOfBoundsException that occurred in EoSyntax when debug logging is enabled by correcting a format string mismatch in the debug log statement.
- Removed an extra
%sformat specifier from the debug log message to match the number of arguments provided - Added a test case to verify the fix works correctly in debug mode
- Updated test dependencies to include necessary logging libraries
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| eo-parser/src/main/java/org/eolang/parser/EoSyntax.java | Fixed format string in debug log statement by removing extraneous %s specifier that was causing ArrayIndexOutOfBoundsException |
| eo-parser/src/test/java/org/eolang/parser/EoSyntaxTest.java | Added test case parsesSimpleCodeWithDebugMode() to verify parsing works correctly when debug logging is enabled |
| eo-parser/pom.xml | Added log4j and reload4j dependencies with test scope to support the new test case |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| syntax::parsed, | ||
| "EO syntax should not fail in debug mode when program has errors" | ||
| ); | ||
| logger.setLevel(before); |
There was a problem hiding this comment.
According to the project's coding guidelines, each test should end with an assertion with no code after it. The logger level restoration logger.setLevel(before) should be handled differently, such as using a try-finally block or JUnit's @AfterEach method to ensure proper cleanup without violating the test structure guideline.
There was a problem hiding this comment.
@volodya-lombrozo this commetn of Copilot does make sense. Tests should not clean after themselves. Instead, they should only clean what's necessary before they start their execution.
| <groupId>ch.qos.reload4j</groupId> | ||
| <artifactId>reload4j</artifactId> | ||
| <version>1.2.22</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
The reload4j dependency appears to be redundant because slf4j-reload4j (lines 104-108) already includes reload4j as a transitive dependency. Adding this explicit dependency could lead to version conflicts or confusion. If a specific version is required, it would be better to use dependency management to control the transitive version rather than adding a duplicate dependency.
| <groupId>ch.qos.reload4j</groupId> | |
| <artifactId>reload4j</artifactId> | |
| <version>1.2.22</version> | |
| <scope>test</scope> | |
| </dependency> | |
| <dependency> |
There was a problem hiding this comment.
I would like to do it, however, I'm getting the following warning from qulice:
[WARNING] Used undeclared dependencies found:
ch.qos.reload4j:reload4j:jar:1.2.22:test
[INFO] You can suppress this message by <exclude>dependencies:...</exclude> in pom.xml, where <...> is what the dependency name starts with (not a regular expression!)
|
@yegor256 would you mind having a look? |
4079ba8 to
2e4fb89
Compare
|
@yegor256 could you take a look one more time, please? |
This PR fixes an
ArrayIndexOutOfBoundsExceptioninEoSyntaxwhen debug mode is enabled.Fixes #4708
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.