Skip to content

Commit d887359

Browse files
liachliach
committed
8370976: Review the behavioral changes of core reflection descriptor parsing migration
Reviewed-by: rriggs, jvernee
1 parent 0972ba6 commit d887359

File tree

7 files changed

+137
-37
lines changed

7 files changed

+137
-37
lines changed

‎src/java.base/share/classes/java/lang/Class.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import java.lang.reflect.Field;
4444
import java.lang.reflect.GenericArrayType;
4545
import java.lang.reflect.GenericDeclaration;
46+
import java.lang.reflect.GenericSignatureFormatError;
4647
import java.lang.reflect.InvocationTargetException;
4748
import java.lang.reflect.Member;
4849
import java.lang.reflect.Method;
@@ -1445,7 +1446,6 @@ public Method getEnclosingMethod() {
14451446
if (!enclosingInfo.isMethod())
14461447
return null;
14471448

1448-
// Descriptor already validated by VM
14491449
List<Class<?>> types = BytecodeDescriptor.parseMethod(enclosingInfo.getDescriptor(), getClassLoader());
14501450
Class<?> returnType = types.removeLast();
14511451
Class<?>[] parameterClasses = types.toArray(EMPTY_CLASS_ARRAY);
@@ -1533,8 +1533,15 @@ boolean isPartial() {
15331533

15341534
String getName() { return name; }
15351535

1536-
String getDescriptor() { return descriptor; }
1537-
1536+
String getDescriptor() {
1537+
// hotspot validates this descriptor to be either a field or method
1538+
// descriptor as the "type" in a NameAndType in verification.
1539+
// So this can still be a field descriptor
1540+
if (descriptor.isEmpty() || descriptor.charAt(0) != '(') {
1541+
throw new GenericSignatureFormatError("Bad method signature: " + descriptor);
1542+
}
1543+
return descriptor;
1544+
}
15381545
}
15391546

15401547
private static Class<?> toClass(Type o) {
@@ -1567,7 +1574,6 @@ public Constructor<?> getEnclosingConstructor() {
15671574
if (!enclosingInfo.isConstructor())
15681575
return null;
15691576

1570-
// Descriptor already validated by VM
15711577
List<Class<?>> types = BytecodeDescriptor.parseMethod(enclosingInfo.getDescriptor(), getClassLoader());
15721578
types.removeLast();
15731579
Class<?>[] parameterClasses = types.toArray(EMPTY_CLASS_ARRAY);

‎src/java.base/share/classes/java/lang/invoke/MethodType.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1194,10 +1194,6 @@ public static MethodType fromMethodDescriptorString(String descriptor, ClassLoad
11941194
static MethodType fromDescriptor(String descriptor, ClassLoader loader)
11951195
throws IllegalArgumentException, TypeNotPresentException
11961196
{
1197-
if (!descriptor.startsWith("(") || // also generates NPE if needed
1198-
descriptor.indexOf(')') < 0 ||
1199-
descriptor.indexOf('.') >= 0)
1200-
throw newIllegalArgumentException("not a method descriptor: "+descriptor);
12011197
List<Class<?>> types = BytecodeDescriptor.parseMethod(descriptor, loader);
12021198
Class<?> rtype = types.remove(types.size() - 1);
12031199
Class<?>[] ptypes = listToArray(types);

‎src/java.base/share/classes/sun/invoke/util/BytecodeDescriptor.java‎

Lines changed: 42 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -63,33 +63,24 @@ public static Class<?> parseClass(String descriptor, ClassLoader loader) {
6363
/// @throws TypeNotPresentException if a reference type cannot be found by
6464
/// the loader (before the descriptor is found invalid)
6565
public static List<Class<?>> parseMethod(String descriptor, ClassLoader loader) {
66-
return parseMethod(descriptor, 0, descriptor.length(), loader);
67-
}
68-
69-
/**
70-
* @param loader the class loader in which to look up the types (null means
71-
* bootstrap class loader)
72-
*/
73-
static List<Class<?>> parseMethod(String bytecodeSignature,
74-
int start, int end, ClassLoader loader) {
75-
String str = bytecodeSignature;
76-
int[] i = {start};
66+
int end = descriptor.length(); // implicit null check
67+
int[] i = {0};
7768
var ptypes = new ArrayList<Class<?>>();
78-
if (i[0] < end && str.charAt(i[0]) == '(') {
69+
if (i[0] < end && descriptor.charAt(i[0]) == '(') {
7970
++i[0]; // skip '('
80-
while (i[0] < end && str.charAt(i[0]) != ')') {
81-
Class<?> pt = parseSig(str, i, end, loader);
71+
while (i[0] < end && descriptor.charAt(i[0]) != ')') {
72+
Class<?> pt = parseSig(descriptor, i, end, loader);
8273
if (pt == null || pt == void.class)
83-
parseError(str, "bad argument type");
74+
parseError(descriptor, "bad argument type");
8475
ptypes.add(pt);
8576
}
8677
++i[0]; // skip ')'
8778
} else {
88-
parseError(str, "not a method type");
79+
parseError(descriptor, "not a method type");
8980
}
90-
Class<?> rtype = parseSig(str, i, end, loader);
81+
Class<?> rtype = parseSig(descriptor, i, end, loader);
9182
if (rtype == null || i[0] != end)
92-
parseError(str, "bad return type");
83+
parseError(descriptor, "bad return type");
9384
ptypes.add(rtype);
9485
return ptypes;
9586
}
@@ -115,8 +106,22 @@ private static Class<?> parseSig(String str, int[] i, int end, ClassLoader loade
115106
if (i[0] == end) return null;
116107
char c = str.charAt(i[0]++);
117108
if (c == 'L') {
118-
int begc = i[0], endc = str.indexOf(';', begc);
119-
if (endc < 0) return null;
109+
int begc = i[0];
110+
int identifierStart = begc;
111+
int endc;
112+
while (true) {
113+
int next = nextNonIdentifier(str, identifierStart, end);
114+
if (identifierStart == next || next >= end) return null; // Empty name segment, or the end
115+
char ch = str.charAt(next);
116+
if (ch == ';') {
117+
endc = next;
118+
break;
119+
} else if (ch == '/') {
120+
identifierStart = next + 1; // Next name segment
121+
} else {
122+
return null; // Bad char [ or .
123+
}
124+
}
120125
i[0] = endc+1;
121126
String name = str.substring(begc, endc).replace('/', '.');
122127
try {
@@ -148,6 +153,23 @@ private static Class<?> parseSig(String str, int[] i, int end, ClassLoader loade
148153
}
149154
}
150155

156+
private static final int CHECK_OFFSET = 32;
157+
private static final long NON_IDENTIFIER_MASK = (1L << ('.' - CHECK_OFFSET))
158+
| (1L << ('/' - CHECK_OFFSET))
159+
| (1L << (';' - CHECK_OFFSET))
160+
| (1L << ('[' - CHECK_OFFSET));
161+
162+
private static int nextNonIdentifier(String str, int index, int end) {
163+
while (index < end) {
164+
int check = str.charAt(index) - CHECK_OFFSET;
165+
if ((check & -Long.SIZE) == 0 && (NON_IDENTIFIER_MASK & (1L << check)) != 0) {
166+
break;
167+
}
168+
index++;
169+
}
170+
return index;
171+
}
172+
151173
public static String unparse(Class<?> type) {
152174
if (type == Object.class) {
153175
return "Ljava/lang/Object;";

‎test/jdk/java/lang/Class/getEnclosingMethod/BadEnclosingMethodTest.java‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434

3535
import java.lang.classfile.ClassFile;
3636
import java.lang.classfile.attribute.EnclosingMethodAttribute;
37+
import java.lang.reflect.GenericSignatureFormatError;
3738
import java.nio.file.Files;
3839
import java.nio.file.Path;
3940
import java.util.Map;
@@ -93,8 +94,16 @@ private Class<?> loadTestClass(String name, String type) throws Exception {
9394
*/
9495
@Test
9596
void testMalformedTypes() throws Exception {
96-
assertThrows(ClassFormatError.class, () -> loadTestClass("methodName", "(L[;)V"));
97-
assertThrows(ClassFormatError.class, () -> loadTestClass(INIT_NAME, "(L[;)V"));
97+
assertThrowsExactly(ClassFormatError.class, () -> loadTestClass("methodName", "(L[;)V"));
98+
assertThrowsExactly(ClassFormatError.class, () -> loadTestClass(INIT_NAME, "(L[;)V"));
99+
assertThrowsExactly(ClassFormatError.class, () -> loadTestClass(INIT_NAME, "(Lmissing/;)V"));
100+
assertThrowsExactly(ClassFormatError.class, () -> loadTestClass(INIT_NAME, "()L/Missing;"));
101+
// only throw if the query type match the method/constructor type
102+
assertDoesNotThrow(() -> loadTestClass(INIT_NAME, "Ljava/lang/Object;").getEnclosingMethod());
103+
assertDoesNotThrow(() -> loadTestClass("method", "[I").getEnclosingConstructor());
104+
// We have to manually intercept field-typed "methods"
105+
assertThrows(GenericSignatureFormatError.class, () -> loadTestClass(INIT_NAME, "Ljava/lang/Object;").getEnclosingConstructor());
106+
assertThrows(GenericSignatureFormatError.class, () -> loadTestClass("method", "[I").getEnclosingMethod());
98107
}
99108

100109
/**

‎test/jdk/java/lang/annotation/MalformedAnnotationTest.java‎

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@
3131

3232
import jdk.test.lib.ByteCodeLoader;
3333
import org.junit.jupiter.api.Test;
34+
import org.junit.jupiter.params.ParameterizedTest;
35+
import org.junit.jupiter.params.provider.MethodSource;
36+
import org.junit.jupiter.params.provider.ValueSource;
3437

3538
import java.lang.annotation.Retention;
3639
import java.lang.annotation.RetentionPolicy;
@@ -41,6 +44,8 @@
4144
import java.lang.classfile.attribute.RuntimeVisibleAnnotationsAttribute;
4245
import java.lang.constant.ClassDesc;
4346
import java.lang.reflect.GenericSignatureFormatError;
47+
import java.util.Arrays;
48+
import java.util.stream.Stream;
4449

4550
import static org.junit.jupiter.api.Assertions.assertThrows;
4651
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -56,22 +61,58 @@ class MalformedAnnotationTest {
5661
Class<?> value();
5762
}
5863

64+
static Stream<String> badFieldDescriptors() {
65+
return Arrays.stream(new String[] {
66+
"Not a_descriptor",
67+
"()V",
68+
"Ljava/lang/Object",
69+
"Ljava/",
70+
"Ljava/util/Map.Entry;",
71+
"[".repeat(256) + "I",
72+
"Lbad.Name;",
73+
"Lbad[Name;",
74+
"L;",
75+
"L/Missing;",
76+
"Lmissing/;",
77+
});
78+
}
79+
5980
/**
6081
* Ensures bad class descriptors in annotations lead to
6182
* {@link GenericSignatureFormatError} and the error message contains the
6283
* malformed descriptor string.
6384
*/
64-
@Test
65-
void testMalformedClassValue() throws Exception {
66-
var badDescString = "Not a_descriptor";
85+
@ParameterizedTest
86+
@MethodSource("badFieldDescriptors")
87+
void testMalformedClassValue(String badDescString) throws Exception {
88+
var cl = spinClass(badDescString);
89+
var ex = assertThrows(GenericSignatureFormatError.class, () -> cl.getDeclaredAnnotation(ClassCarrier.class));
90+
assertTrue(ex.getMessage().contains(badDescString), () -> "Uninformative error: " + ex);
91+
}
92+
93+
private static Class<?> spinClass(String desc) throws Exception {
6794
var bytes = ClassFile.of().build(ClassDesc.of("Test"), clb -> clb
6895
.with(RuntimeVisibleAnnotationsAttribute.of(
6996
Annotation.of(ClassCarrier.class.describeConstable().orElseThrow(),
7097
AnnotationElement.of("value", AnnotationValue.ofClass(clb
71-
.constantPool().utf8Entry(badDescString))))
98+
.constantPool().utf8Entry(desc))))
7299
)));
73-
var cl = new ByteCodeLoader("Test", bytes, MalformedAnnotationTest.class.getClassLoader()).loadClass("Test");
74-
var ex = assertThrows(GenericSignatureFormatError.class, () -> cl.getDeclaredAnnotation(ClassCarrier.class));
75-
assertTrue(ex.getMessage().contains(badDescString), () -> "Uninformative error: " + ex);
100+
return new ByteCodeLoader("Test", bytes, ClassCarrier.class.getClassLoader()).loadClass("Test");
101+
}
102+
103+
static Stream<String> goodFieldDescriptors() {
104+
return Arrays.stream(new String[] {
105+
"Ljava/lang/Object<*>;", // previously MalformedParameterizedTypeException
106+
"[Ljava/util/Optional<*>;", // previously ClassCastException
107+
"Ljava/util/Map$Entry<**>;", // previously ClassCastException
108+
});
109+
}
110+
111+
@ParameterizedTest
112+
@MethodSource("goodFieldDescriptors")
113+
void testLegalClassValue(String goodDescString) throws Exception {
114+
var cl = spinClass(goodDescString);
115+
var anno = cl.getDeclaredAnnotation(ClassCarrier.class);
116+
assertThrows(TypeNotPresentException.class, anno::value);
76117
}
77118
}

‎test/jdk/java/lang/invoke/MethodTypeTest.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,11 @@ public String[] badMethodDescriptorStrings() {
230230
"(" + "[".repeat(256) + "J)I",
231231
"(java/lang/Object)V",
232232
"()java/lang/Object",
233+
"()Lbad.Name;",
234+
"()Lbad[Name;",
235+
"(L;)V",
236+
"(L/Missing;)I",
237+
"(Lmissing/;)Z",
233238
};
234239
}
235240

‎test/jdk/sun/invoke/util/BytecodeDescriptorTest.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,13 @@ void testParseClass() throws ReflectiveOperationException {
7979
assertSame(int.class, BytecodeDescriptor.parseClass("I", null), "primitive");
8080
assertSame(long[][].class, BytecodeDescriptor.parseClass("[[J", null), "array");
8181
assertSame(Object.class, BytecodeDescriptor.parseClass("Ljava/lang/Object;", null), "class or interface");
82+
assertThrows(IllegalArgumentException.class, () -> BytecodeDescriptor.parseClass("java.lang.Object", null), "binary name");
83+
assertThrows(IllegalArgumentException.class, () -> BytecodeDescriptor.parseClass("L[a;", null), "bad class or interface");
84+
assertThrows(IllegalArgumentException.class, () -> BytecodeDescriptor.parseClass("Ljava.lang.Object;", null), "bad class or interface");
8285
assertThrows(IllegalArgumentException.class, () -> BytecodeDescriptor.parseClass("java/lang/Object", null), "internal name");
86+
assertThrows(IllegalArgumentException.class, () -> BytecodeDescriptor.parseClass("L;", null), "empty name");
87+
assertThrows(IllegalArgumentException.class, () -> BytecodeDescriptor.parseClass("Lmissing/;", null), "empty name part");
88+
assertThrows(IllegalArgumentException.class, () -> BytecodeDescriptor.parseClass("L/Missing;", null), "empty name part");
8389
assertThrows(IllegalArgumentException.class, () -> BytecodeDescriptor.parseClass("[V", null), "bad array");
8490
assertSame(Class.forName("[".repeat(255) + "I"), BytecodeDescriptor.parseClass("[".repeat(255) + "I", null), "good array");
8591
assertThrows(IllegalArgumentException.class, () -> BytecodeDescriptor.parseClass("[".repeat(256) + "I", null), "bad array");
@@ -110,6 +116,21 @@ void testParseMethod() {
110116
assertThrows(IllegalArgumentException.class,
111117
() -> BytecodeDescriptor.parseClass("([".repeat(256) + "I)J", null),
112118
"bad arg");
119+
assertThrows(IllegalArgumentException.class,
120+
() -> BytecodeDescriptor.parseMethod("(Ljava.lang.Object;)V", null),
121+
"bad arg");
122+
assertThrows(IllegalArgumentException.class,
123+
() -> BytecodeDescriptor.parseMethod("(Ljava/lang[Object;)V", null),
124+
"bad arg");
125+
assertThrows(IllegalArgumentException.class,
126+
() -> BytecodeDescriptor.parseMethod("(L;)V", null),
127+
"bad arg");
128+
assertThrows(IllegalArgumentException.class,
129+
() -> BytecodeDescriptor.parseMethod("(Ljava/lang/;)V", null),
130+
"bad arg");
131+
assertThrows(IllegalArgumentException.class,
132+
() -> BytecodeDescriptor.parseMethod("(L/Object;)V", null),
133+
"bad arg");
113134

114135
assertEquals(List.of(foo1, bar1),
115136
BytecodeDescriptor.parseMethod("(" + FOO_DESC + ")" + BAR_DESC, cl1),

0 commit comments

Comments
 (0)