Skip to content

Commit

Permalink
Fixes potential StringIndexOutOfBoundsException in Java parser (#5003)
Browse files Browse the repository at this point in the history 
* 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 <tim@moderne.io>
Co-authored-by: lingenj <jacob.van.lingen@moderne.io>
  • Loading branch information
@knutwannheden @timtebeek @jevanlingen
3 people authored Feb 10, 2025
1 parent efb5882 commit c50c746
Show file tree
Hide file tree
Showing 5 changed files with 96 additions and 75 deletions.
32 changes: 15 additions & 17 deletions ...te-java-11/src/main/java/org/openrewrite/java/isolated/ReloadableJava11ParserVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 <J2 extends @Nullable J> @Nullable JRightPadded<J2> convert(@Nullable Tree t, Function<Tree, Space> suffix) {
return convert(t, suffix, j -> Markers.EMPTY);
}
Expand Down Expand Up @@ -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")
Expand Down
41 changes: 17 additions & 24 deletions ...te-java-17/src/main/java/org/openrewrite/java/isolated/ReloadableJava17ParserVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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:");
Expand Down Expand Up @@ -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")
Expand Down
32 changes: 15 additions & 17 deletions ...te-java-21/src/main/java/org/openrewrite/java/isolated/ReloadableJava21ParserVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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);
Expand All @@ -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:");
Expand Down Expand Up @@ -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")
Expand Down
32 changes: 15 additions & 17 deletions rewrite-java-8/src/main/java/org/openrewrite/java/ReloadableJava8ParserVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 <J2 extends @Nullable J> @Nullable JRightPadded<J2> convert(@Nullable Tree t, Function<Tree, Space> suffix) {
if (t == null) {
return null;
Expand Down Expand Up @@ -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")
Expand Down
34 changes: 34 additions & 0 deletions rewrite-java-tck/src/main/java/org/openrewrite/java/tree/LombokTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit c50c746

Please sign in to comment.