8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures

Reviewed-by: jpai

Author: asotona

src/java.base/share/classes/jdk/internal/classfile/impl/SignaturesImpl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -44,49 +44,64 @@ public SignaturesImpl() {
     public ClassSignature parseClassSignature(String signature) {
         this.sig = signature;
         sigp = 0;
-        List<TypeParam> typeParamTypes = parseParamTypes();
-        RefTypeSig superclass = referenceTypeSig();
-        ArrayList<RefTypeSig> superinterfaces = null;
-        while (sigp < sig.length()) {
-            if (superinterfaces == null)
-                superinterfaces = new ArrayList<>();
-            superinterfaces.add(referenceTypeSig());
+        try {
+            List<TypeParam> typeParamTypes = parseParamTypes();
+            RefTypeSig superclass = referenceTypeSig();
+            ArrayList<RefTypeSig> superinterfaces = null;
+            while (sigp < sig.length()) {
+                if (superinterfaces == null)
+                    superinterfaces = new ArrayList<>();
+                superinterfaces.add(referenceTypeSig());
+            }
+            return new ClassSignatureImpl(typeParamTypes, superclass, null2Empty(superinterfaces));
+        } catch (IndexOutOfBoundsException e) {
+            throw error("Not a valid class signature");
         }
-        return new ClassSignatureImpl(typeParamTypes, superclass, null2Empty(superinterfaces));
     }
 
     public MethodSignature parseMethodSignature(String signature) {
         this.sig = signature;
         sigp = 0;
-        List<TypeParam> typeParamTypes = parseParamTypes();
-        assert sig.charAt(sigp) == '(';
-        sigp++;
-        ArrayList<Signature> paramTypes = null;
-        while (sig.charAt(sigp) != ')') {
-            if (paramTypes == null)
-                 paramTypes = new ArrayList<>();
-            paramTypes.add(typeSig());
-        }
-        sigp++;
-        Signature returnType = typeSig();
-        ArrayList<ThrowableSig> throwsTypes = null;
-        while (sigp < sig.length() && sig.charAt(sigp) == '^') {
+        try {
+            List<TypeParam> typeParamTypes = parseParamTypes();
+            if (sig.charAt(sigp) != '(') throw error("Expected ( at possition %d of signature".formatted(sigp));
+            sigp++;
+            ArrayList<Signature> paramTypes = null;
+            while (sig.charAt(sigp) != ')') {
+                if (paramTypes == null)
+                     paramTypes = new ArrayList<>();
+                paramTypes.add(typeSig());
+            }
             sigp++;
-            if (throwsTypes == null)
-                throwsTypes = new ArrayList<>();
-            var t = typeSig();
-            if (t instanceof ThrowableSig ts)
-                throwsTypes.add(ts);
-            else
-                throw new IllegalArgumentException("not a valid type signature: " + sig);
+            Signature returnType = typeSig();
+            ArrayList<ThrowableSig> throwsTypes = null;
+            while (sigp < sig.length()) {
+                if (sig.charAt(sigp) != '^') throw error("Expected ^ at possition %d of signature".formatted(sigp));
+                sigp++;
+                if (throwsTypes == null)
+                    throwsTypes = new ArrayList<>();
+                var t = referenceTypeSig();
+                if (t instanceof ThrowableSig ts)
+                    throwsTypes.add(ts);
+                else
+                    throw error("Not a valid throwable signature %s in".formatted(t.signatureString()));
+            }
+            return new MethodSignatureImpl(typeParamTypes, null2Empty(throwsTypes), returnType, null2Empty(paramTypes));
+        } catch (IndexOutOfBoundsException e) {
+            throw error("Not a valid method signature");
         }
-        return new MethodSignatureImpl(typeParamTypes, null2Empty(throwsTypes), returnType, null2Empty(paramTypes));
     }
 
     public Signature parseSignature(String signature) {
         this.sig = signature;
         sigp = 0;
-        return typeSig();
+        try {
+            var s = typeSig();
+            if (sigp == signature.length())
+                return s;
+        } catch (IndexOutOfBoundsException e) {
+        }
+        throw error("Not a valid type signature");
     }
 
     private List<TypeParam> parseParamTypes() {
@@ -157,7 +172,7 @@ private RefTypeSig referenceTypeSig() {
                 return ty;
             case '[': return ArrayTypeSig.of(typeSig());
         }
-        throw new IllegalArgumentException("not a valid type signature: " + sig);
+        throw error("Unexpected character %c at possition %d of signature".formatted(c, sigp - 1));
     }
 
     private TypeArg typeArg() {
@@ -292,4 +307,8 @@ public String signatureString() {
     private static <T> List<T> null2Empty(ArrayList<T> l) {
         return l == null ? List.of() : Collections.unmodifiableList(l);
     }
+
+    private IllegalArgumentException error(String message) {
+        return new IllegalArgumentException("%s: %s".formatted(message, sig));
+    }
 }

test/jdk/jdk/classfile/SignaturesTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -24,6 +24,7 @@
 /*
  * @test
  * @summary Testing Signatures.
+ * @bug 8321540
  * @run junit SignaturesTest
  */
 import java.io.IOException;
@@ -46,8 +47,11 @@
 import java.lang.classfile.Attributes;
 import org.junit.jupiter.api.Test;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import org.junit.jupiter.api.Assertions;
 import static helpers.ClassRecord.assertEqualsDeep;
 import static java.lang.constant.ConstantDescs.*;
+import java.util.function.Consumer;
+import java.util.function.Function;
 
 class SignaturesTest {
 
@@ -186,4 +190,79 @@ void testClassSignatureClassDesc() throws IOException {
         assertEquals(Outer.Inner.class.describeConstable().orElseThrow(), innerSig.classDesc(),
                 "ClassDesc derived from signature");
     }
+
+    @Test
+    void testBadTypeSignatures() {
+        """
+        LObject
+        LObject;B
+        LIterable<LFoo>
+        LIterable<<
+        TBar
+        TBar<LFoo;>
+        B<LFoo;>
+        B<LFoo;>;V
+        X
+        [LObject
+        [LIterable<LFoo>
+        [LIterable<<
+        [TBar
+        [TBar<LFoo;>
+        [B<LFoo;>
+        [X
+        LSet<+Kind<**>;>;
+        LSet<?Kind<*>;>;
+        ()V
+        """.lines().forEach(assertThrows(Signature::parseFrom));
+    }
+
+    @Test
+    void testBadClassSignatures() {
+        """
+        Ljava/lang/Object;Ljava/lang/Iterable<LFoo;>
+        LObject
+        LObject;B
+        LIterable<LFoo>
+        LIterable<<
+        TBar
+        TBar<LFoo;>
+        B<LFoo;>
+        B<LFoo;>;V
+        X
+        LFoo<TK;>.It;L
+        <K+LObject;>LFoo<TK;;>;LFoo<TK;>;LBar;
+        <K:LObject;>>LFoo<TK;>;
+        <K:LObject;>LFoo<+>;
+        ()V
+        """.lines().forEach(assertThrows(ClassSignature::parseFrom));
+    }
+
+    @Test
+    void testBadMethodSignatures() {
+        """
+        LObject;
+        B
+        ()V^
+        ()V^B
+        ()V^X
+        (LObject;)
+        (LObject)V
+        ()LIterable<LFoo>
+        ()LIterable<<
+        ()TBar
+        ()TBar;B
+        (TBar<LFoo;>)V
+        (B<LFoo;>)V
+        (X)
+        ()X
+        ()VB
+        ()LSet<+Kind<**>;>;
+        (LSet<?Kind<*>;>;)V
+        <T::LA>()V
+        """.lines().forEach(assertThrows(MethodSignature::parseFrom));
+    }
+
+    private Consumer<String> assertThrows(Function<String, ?> parser) {
+        return s -> Assertions.assertThrows(IllegalArgumentException.class, () -> parser.apply(s), s);
+    }
 }