From c50c7460974f2d4ffcf657ee084545901f207a82 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Mon, 10 Feb 2025 16:59:53 +0100 Subject: [PATCH] Fixes potential `StringIndexOutOfBoundsException` in Java parser (#5003) * Fixes potential `StringIndexOutOfBoundsException` in Java parser ``` java.lang.StringIndexOutOfBoundsException: begin 1495, end 1475, length 3807 java.base/java.lang.String.checkBoundsBeginEnd(String.java:4606) java.base/java.lang.String.substring(String.java:2709) org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitAnnotation(ReloadableJava17ParserVisitor.java:151) org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitAnnotation(ReloadableJava17ParserVisitor.java:77) com.sun.tools.javac.tree.JCTree$JCAnnotation.accept(JCTree.java:2921) com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86) org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert(ReloadableJava17ParserVisitor.java:1746) org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.sortedModifiersAndAnnotations(ReloadableJava17ParserVisitor.java:2187) org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitVariables(ReloadableJava17ParserVisitor.java:1593) org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitVariable(ReloadableJava17ParserVisitor.java:1583) org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitVariable(ReloadableJava17ParserVisitor.java:77) com.sun.tools.javac.tree.JCTree$JCVariableDecl.accept(JCTree.java:1045) com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86) org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert(ReloadableJava17ParserVisitor.java:1746) org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert(ReloadableJava17ParserVisitor.java:1797) org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert(ReloadableJava17ParserVisitor.java:1790) ... ``` * Push reproducer test * Show that the problem is unique to `@Value` handling * Test constructors as well, and With * Implementation * Restore JavaDoc types * Use java 1.5 format function * Exclude prefix handling for enums in convert method * Exclude prefix handling for enums in convert method * Add comment --------- Co-authored-by: Tim te Beek Co-authored-by: lingenj --- .../ReloadableJava11ParserVisitor.java | 32 +++++++-------- .../ReloadableJava17ParserVisitor.java | 41 ++++++++----------- .../ReloadableJava21ParserVisitor.java | 32 +++++++-------- .../java/ReloadableJava8ParserVisitor.java | 32 +++++++-------- .../org/openrewrite/java/tree/LombokTest.java | 34 +++++++++++++++ 5 files changed, 96 insertions(+), 75 deletions(-) diff --git a/rewrite-java-11/src/main/java/org/openrewrite/java/isolated/ReloadableJava11ParserVisitor.java b/rewrite-java-11/src/main/java/org/openrewrite/java/isolated/ReloadableJava11ParserVisitor.java index b1e38e95402..9ea04496033 100644 --- a/rewrite-java-11/src/main/java/org/openrewrite/java/isolated/ReloadableJava11ParserVisitor.java +++ b/rewrite-java-11/src/main/java/org/openrewrite/java/isolated/ReloadableJava11ParserVisitor.java @@ -141,17 +141,17 @@ public J visitAnnotation(AnnotationTree node, Space fmt) { args = JContainer.build(argsPrefix, expressions, Markers.EMPTY); } else { - String remaining = source.substring(cursor, endPos(node)); - - // TODO: technically, if there is code like this, we have a bug, but seems exceedingly unlikely: - // @MyAnnotation /* Comment () that contains parentheses */ () - - if (remaining.contains("(") && remaining.contains(")")) { + int saveCursor = cursor; + Space prefix = whitespace(); + if (source.charAt(cursor) == '(') { + skip("("); args = JContainer.build( - sourceBefore("("), + prefix, singletonList(padRight(new J.Empty(randomId(), sourceBefore(")"), Markers.EMPTY), EMPTY)), Markers.EMPTY ); + } else { + cursor = saveCursor; } } @@ -1655,7 +1655,9 @@ public J visitWildcard(WildcardTree node, Space fmt) { return null; } try { - String prefix = source.substring(cursor, max(cursor, getActualStartPosition((JCTree) t))); + // The spacing of initialized enums such as `ONE (1)` is handled in the `visitNewClass` method, so set it explicitly to “” here. + String prefix = t instanceof JCNewClass && hasFlag(((JCNewClass) t).type.tsym.flags(), Flags.ENUM) ? "" + : source.substring(cursor, indexOfNextNonWhitespace(cursor, source)); cursor += prefix.length(); @SuppressWarnings("unchecked") J2 j = (J2) scan(t, formatWithCommentTree(prefix, (JCTree) t, docCommentTable.getCommentTree((JCTree) t))); return j; @@ -1685,14 +1687,6 @@ public J visitWildcard(WildcardTree node, Space fmt) { } } - private static int getActualStartPosition(JCTree t) { - // The variable's start position in the source is wrongly after lombok's `@val` annotation - if (t instanceof JCVariableDecl && isLombokGenerated(t)) { - return ((JCVariableDecl) t).mods.annotations.get(0).getStartPosition(); - } - return t.getStartPosition(); - } - private @Nullable JRightPadded convert(@Nullable Tree t, Function suffix) { return convert(t, suffix, j -> Markers.EMPTY); } @@ -2043,7 +2037,11 @@ private void cursor(int n) { } private boolean hasFlag(ModifiersTree modifiers, long flag) { - return (((JCModifiers) modifiers).flags & flag) != 0L; + return hasFlag(((JCModifiers) modifiers).flags, flag); + } + + private boolean hasFlag(long flags, long flag) { + return (flags & flag) != 0L; } @SuppressWarnings("unused") diff --git a/rewrite-java-17/src/main/java/org/openrewrite/java/isolated/ReloadableJava17ParserVisitor.java b/rewrite-java-17/src/main/java/org/openrewrite/java/isolated/ReloadableJava17ParserVisitor.java index f1eab81c6b4..1a76427b974 100644 --- a/rewrite-java-17/src/main/java/org/openrewrite/java/isolated/ReloadableJava17ParserVisitor.java +++ b/rewrite-java-17/src/main/java/org/openrewrite/java/isolated/ReloadableJava17ParserVisitor.java @@ -148,17 +148,17 @@ public J visitAnnotation(AnnotationTree node, Space fmt) { args = JContainer.build(argsPrefix, expressions, Markers.EMPTY); } else { - String remaining = source.substring(cursor, endPos(node)); - - // TODO: technically, if there is code like this, we have a bug, but seems exceedingly unlikely: - // @MyAnnotation /* Comment () that contains parentheses */ () - - if (remaining.contains("(") && remaining.contains(")")) { + int saveCursor = cursor; + Space prefix = whitespace(); + if (source.charAt(cursor) == '(') { + skip("("); args = JContainer.build( - sourceBefore("("), + prefix, singletonList(padRight(new J.Empty(randomId(), sourceBefore(")"), Markers.EMPTY), EMPTY)), Markers.EMPTY ); + } else { + cursor = saveCursor; } } @@ -1738,30 +1738,19 @@ public J visitWildcard(WildcardTree node, Space fmt) { return null; } try { - String prefix = source.substring(cursor, Math.max(cursor, getActualStartPosition((JCTree) t))); + // The spacing of initialized enums such as `ONE (1)` is handled in the `visitNewClass` method, so set it explicitly to “” here. + String prefix = t instanceof JCNewClass && hasFlag(((JCNewClass) t).type.tsym.flags(), Flags.ENUM) ? "" + : source.substring(cursor, indexOfNextNonWhitespace(cursor, source)); cursor += prefix.length(); - // Java 21 and 23 have a different return type from getCommentTree; with reflection we can support both - Method getCommentTreeMethod = DocCommentTable.class.getMethod("getCommentTree", JCTree.class); - DocCommentTree commentTree = (DocCommentTree) getCommentTreeMethod.invoke(docCommentTable, t); - @SuppressWarnings("unchecked") J2 j = (J2) scan(t, formatWithCommentTree(prefix, (JCTree) t, commentTree)); + Space p = formatWithCommentTree(prefix, (JCTree) t, docCommentTable.getCommentTree((JCTree) t)); + @SuppressWarnings("unchecked") J2 j = (J2) scan(t, p); return j; - } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException ex) { - reportJavaParsingException(ex); - throw new IllegalStateException("Failed to invoke getCommentTree method", ex); } catch (Throwable ex) { reportJavaParsingException(ex); throw ex; } } - private static int getActualStartPosition(JCTree t) { - // The variable's start position in the source is wrongly after lombok's `@val` annotation - if (t instanceof JCVariableDecl && isLombokGenerated(t)) { - return ((JCVariableDecl) t).mods.annotations.get(0).getStartPosition(); - } - return t.getStartPosition(); - } - private void reportJavaParsingException(Throwable ex) { // this SHOULD never happen, but is here simply as a diagnostic measure in the event of unexpected exceptions StringBuilder message = new StringBuilder("Failed to convert for the following cursor stack:"); @@ -2129,7 +2118,11 @@ private void cursor(int n) { } private boolean hasFlag(ModifiersTree modifiers, long flag) { - return (((JCModifiers) modifiers).flags & flag) != 0L; + return hasFlag(((JCModifiers) modifiers).flags, flag); + } + + private boolean hasFlag(long flags, long flag) { + return (flags & flag) != 0L; } @SuppressWarnings("unused") diff --git a/rewrite-java-21/src/main/java/org/openrewrite/java/isolated/ReloadableJava21ParserVisitor.java b/rewrite-java-21/src/main/java/org/openrewrite/java/isolated/ReloadableJava21ParserVisitor.java index 6815dfe3262..bba5e6e49fb 100644 --- a/rewrite-java-21/src/main/java/org/openrewrite/java/isolated/ReloadableJava21ParserVisitor.java +++ b/rewrite-java-21/src/main/java/org/openrewrite/java/isolated/ReloadableJava21ParserVisitor.java @@ -148,17 +148,17 @@ public J visitAnnotation(AnnotationTree node, Space fmt) { args = JContainer.build(argsPrefix, expressions, Markers.EMPTY); } else { - String remaining = source.substring(cursor, endPos(node)); - - // TODO: technically, if there is code like this, we have a bug, but seems exceedingly unlikely: - // @MyAnnotation /* Comment () that contains parentheses */ () - - if (remaining.contains("(") && remaining.contains(")")) { + int saveCursor = cursor; + Space prefix = whitespace(); + if (source.charAt(cursor) == '(') { + skip("("); args = JContainer.build( - sourceBefore("("), + prefix, singletonList(padRight(new J.Empty(randomId(), sourceBefore(")"), Markers.EMPTY), EMPTY)), Markers.EMPTY ); + } else { + cursor = saveCursor; } } @@ -1766,7 +1766,9 @@ public J visitWildcard(WildcardTree node, Space fmt) { return null; } try { - String prefix = source.substring(cursor, Math.max(cursor, getActualStartPosition((JCTree) t))); + // The spacing of initialized enums such as `ONE (1)` is handled in the `visitNewClass` method, so set it explicitly to “” here. + String prefix = t instanceof JCNewClass && hasFlag(((JCNewClass) t).type.tsym.flags(), Flags.ENUM) ? "" + : source.substring(cursor, indexOfNextNonWhitespace(cursor, source)); cursor += prefix.length(); // Java 21 and 23 have a different return type from getCommentTree; with reflection we can support both Method getCommentTreeMethod = DocCommentTable.class.getMethod("getCommentTree", JCTree.class); @@ -1782,14 +1784,6 @@ public J visitWildcard(WildcardTree node, Space fmt) { } } - private static int getActualStartPosition(JCTree t) { - // The variable's start position in the source is wrongly after lombok's `@val` annotation - if (t instanceof JCVariableDecl && isLombokGenerated(t)) { - return ((JCVariableDecl) t).mods.annotations.get(0).getStartPosition(); - } - return t.getStartPosition(); - } - private void reportJavaParsingException(Throwable ex) { // this SHOULD never happen, but is here simply as a diagnostic measure in the event of unexpected exceptions StringBuilder message = new StringBuilder("Failed to convert for the following cursor stack:"); @@ -2157,7 +2151,11 @@ private void cursor(int n) { } private boolean hasFlag(ModifiersTree modifiers, long flag) { - return (((JCModifiers) modifiers).flags & flag) != 0L; + return hasFlag(((JCModifiers) modifiers).flags, flag); + } + + private boolean hasFlag(long flags, long flag) { + return (flags & flag) != 0L; } @SuppressWarnings("unused") diff --git a/rewrite-java-8/src/main/java/org/openrewrite/java/ReloadableJava8ParserVisitor.java b/rewrite-java-8/src/main/java/org/openrewrite/java/ReloadableJava8ParserVisitor.java index 56e6141f6c7..14fed31acd8 100644 --- a/rewrite-java-8/src/main/java/org/openrewrite/java/ReloadableJava8ParserVisitor.java +++ b/rewrite-java-8/src/main/java/org/openrewrite/java/ReloadableJava8ParserVisitor.java @@ -138,17 +138,17 @@ public J visitAnnotation(AnnotationTree node, Space fmt) { args = JContainer.build(argsPrefix, expressions, Markers.EMPTY); } else { - String remaining = source.substring(cursor, endPos(node)); - - // TODO: technically, if there is code like this, we have a bug, but seems exceedingly unlikely: - // @MyAnnotation /* Comment () that contains parentheses */ () - - if (remaining.contains("(") && remaining.contains(")")) { + int saveCursor = cursor; + Space prefix = whitespace(); + if (source.charAt(cursor) == '(') { + skip("("); args = JContainer.build( - sourceBefore("("), + prefix, singletonList(padRight(new J.Empty(randomId(), sourceBefore(")"), Markers.EMPTY), EMPTY)), Markers.EMPTY ); + } else { + cursor = saveCursor; } } @@ -1639,7 +1639,9 @@ public J visitWildcard(WildcardTree node, Space fmt) { return null; } try { - String prefix = source.substring(cursor, Math.max(cursor, getActualStartPosition((JCTree) t))); + // The spacing of initialized enums such as `ONE (1)` is handled in the `visitNewClass` method, so set it explicitly to “” here. + String prefix = t instanceof JCNewClass && hasFlag(((JCNewClass) t).type.tsym.flags(), Flags.ENUM) ? "" + : source.substring(cursor, indexOfNextNonWhitespace(cursor, source)); cursor += prefix.length(); @SuppressWarnings("unchecked") J2 j = (J2) scan(t, formatWithCommentTree(prefix, (JCTree) t, docCommentTable.getCommentTree((JCTree) t))); return j; @@ -1669,14 +1671,6 @@ public J visitWildcard(WildcardTree node, Space fmt) { } } - private static int getActualStartPosition(JCTree t) { - // not sure if this is a bug in Lombok, but the variable's start position is after the `val` annotation - if (t instanceof JCVariableDecl && isLombokGenerated(t)) { - return ((JCVariableDecl) t).mods.annotations.get(0).getStartPosition(); - } - return t.getStartPosition(); - } - private @Nullable JRightPadded convert(@Nullable Tree t, Function suffix) { if (t == null) { return null; @@ -2039,7 +2033,11 @@ private void cursor(int n) { } private boolean hasFlag(ModifiersTree modifiers, long flag) { - return (((JCModifiers) modifiers).flags & flag) != 0L; + return hasFlag(((JCModifiers) modifiers).flags, flag); + } + + private boolean hasFlag(long flags, long flag) { + return (flags & flag) != 0L; } @SuppressWarnings("unused") diff --git a/rewrite-java-tck/src/main/java/org/openrewrite/java/tree/LombokTest.java b/rewrite-java-tck/src/main/java/org/openrewrite/java/tree/LombokTest.java index 746239408ca..530b4c43f03 100644 --- a/rewrite-java-tck/src/main/java/org/openrewrite/java/tree/LombokTest.java +++ b/rewrite-java-tck/src/main/java/org/openrewrite/java/tree/LombokTest.java @@ -19,6 +19,8 @@ import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.openrewrite.java.JavaParser; import org.openrewrite.java.MinimumJava11; import org.openrewrite.java.MinimumJava17; @@ -913,6 +915,38 @@ public void test() { ); } + @ParameterizedTest + @ValueSource(strings = { + "AllArgsConstructor", + "Builder", + "Data", + "EqualsAndHashCode", + "NoArgsConstructor", + "RequiredArgsConstructor", + "ToString", + "Value", + "With" + }) + @MinimumJava11 + void npeSeenOnMultipleAnnotations(String annotation) { + rewriteRun( + spec -> spec.parser(JavaParser.fromJavaVersion().classpath(JavaParser.runtimeClasspath())), + java( + //language=java + String.format(""" + import lombok.%s; + import org.jspecify.annotations.Nullable; + + @%1$s + public class Foo { + @Nullable + String bar; + } + """, annotation) + ) + ); + } + /** * These test lombok features that we do not fully support. * Code should still parse and print back to its original source code but type information may be missing.