8272515: JFR: Names should only be valid Java identifiers

Reviewed-by: mgronlun

Author: egahlin

src/java.base/share/classes/jdk/internal/module/Checks.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2021, 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
@@ -168,7 +168,7 @@ private static String requireTypeName(String what, String name) {
      * Returns true if the given string is a legal Java identifier,
      * otherwise false.
      */
-    private static boolean isJavaIdentifier(String str) {
+    public static boolean isJavaIdentifier(String str) {
         if (str.isEmpty() || RESERVED.contains(str))
             return false;
 

src/jdk.jfr/share/classes/jdk/jfr/EventFactory.java

@@ -33,6 +33,7 @@
 import java.util.Objects;
 import java.util.Set;
 
+import jdk.internal.module.Checks;
 import jdk.jfr.internal.EventClassBuilder;
 import jdk.jfr.internal.JVMSupport;
 import jdk.jfr.internal.MetadataRepository;
@@ -135,7 +136,7 @@ public static EventFactory create(List<AnnotationElement> annotationElements, Li
             if (!Type.isValidJavaFieldType(v.getTypeName())) {
                 throw new IllegalArgumentException(v.getTypeName() + " is not a valid type for an event field");
             }
-            if (!Type.isValidJavaIdentifier(v.getName())) {
+            if (!Checks.isJavaIdentifier(v.getName())) {
                 throw new IllegalArgumentException(name + " is not a valid name for an event field");
             }
             if (nameSet.contains(name)) {

src/jdk.jfr/share/classes/jdk/jfr/Name.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2021, 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
@@ -33,9 +33,13 @@
 /**
  * Annotation that sets the default name for an element.
  * <p>
- * The name must be a valid identifier as specified in the Java language (for
- * example, {@code "com.example.Transaction"} for an event class or
- * {@code "message"} for an event field).
+ * For event classes, the name must be a legal class name as specified in the Java
+ * language, (for example, {@code "com.example.Transaction"}. For event fields
+ * or event settings, the name must be a valid identifier (for example,
+ * {@code "message"}). See section 3.8 and 3.9 of the Java Language
+ * Specification for more information.
+ * <p>
+ * If the specified name is invalid, the annotation is ignored.
  * <p>
  * A stable and easy-to-use event name is of the form:
  * <p>

src/jdk.jfr/share/classes/jdk/jfr/ValueDescriptor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2021, 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
@@ -83,12 +83,14 @@ public final class ValueDescriptor {
      * </ul>
      *
      * <p>
-     * The name must be a valid Java identifier (for example, {@code "maxThroughput"}). See 3.8
-     * Java Language Specification for more information.
+     * The name must be a valid Java identifier (for example, {@code "maxThroughput"}). See
+     * section 3.8 and 3.9 of the Java Language Specification for more information.
      *
      * @param type the type, not {@code null}
      * @param name the name, not {@code null}
      *
+     * @throws IllegalArgumentException if the name is not a valid Java identifier
+     *
      * @throws SecurityException if a security manager is present and the caller
      *         doesn't have {@code FlightRecorderPermission("registerEvent")}
      *
@@ -118,14 +120,16 @@ public ValueDescriptor(Class<?> type, String name) {
      * </ul>
      *
      * <p>
-     * The name must be a valid Java identifier (for example, {@code "maxThroughput"}). See 3.8
-     * Java Language Specification for more information.
+     * The name must be a valid Java identifier (for example, {@code "maxThroughput"}). See
+     * section 3.8 and 3.9 of the Java Language Specification for more information.
      *
      * @param type the type, not {@code null}
      * @param name the name, not {@code null}
      * @param annotations the annotations on the value descriptors, not
      *        {@code null}
      *
+     * @throws IllegalArgumentException if the name is not a valid Java identifier
+     *
      * @throws SecurityException if a security manager is present and the caller
      *         doesn't have {@code FlightRecorderPermission("registerEvent")}
      */
@@ -143,6 +147,7 @@ public ValueDescriptor(Class<?> type, String name, List<AnnotationElement> annot
             }
         }
         this.name = Objects.requireNonNull(name, "Name of value descriptor can't be null");
+        Utils.ensureJavaIdentifier(name);
         this.type = Objects.requireNonNull(Utils.getValidType(Objects.requireNonNull(type), Objects.requireNonNull(name)));
         this.annotationConstruct = new AnnotationConstruct(annotations);
         this.javaFieldName = name; // Needed for dynamic events

src/jdk.jfr/share/classes/jdk/jfr/internal/EventControl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2021, 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
@@ -150,7 +150,7 @@ private void defineSettings(Class<?> eventClass) {
                             String name = m.getName();
                             Name n = m.getAnnotation(Name.class);
                             if (n != null) {
-                                name = n.value();
+                                name = Utils.validJavaIdentifier(n.value(), name);
                             }
 
                             if (!hasControl(name)) {

src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2021, 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
@@ -213,13 +213,23 @@ private static List<SettingInfo> buildSettingInfos(Class<?> superClass, ClassNod
         Set<String> methodSet = new HashSet<>();
         List<SettingInfo> settingInfos = new ArrayList<>();
         String settingDescriptor = Type.getType(SettingDefinition.class).getDescriptor();
+        String nameDescriptor = Type.getType(Name.class).getDescriptor();
         for (MethodNode m : classNode.methods) {
             if (m.visibleAnnotations != null) {
                 for (AnnotationNode an : m.visibleAnnotations) {
                     // We can't really validate the method at this
                     // stage. We would need to check that the parameter
                     // is an instance of SettingControl.
                     if (settingDescriptor.equals(an.desc)) {
+                        String name = m.name;
+                        for (AnnotationNode nameCandidate : m.visibleAnnotations) {
+                            if (nameDescriptor.equals(nameCandidate.desc)) {
+                                List<Object> values = nameCandidate.values;
+                                if (values.size() == 1 && values.get(0) instanceof String s) {
+                                    name = Utils.validJavaIdentifier(s, name);
+                                }
+                            }
+                        }
                         Type returnType = Type.getReturnType(m.desc);
                         if (returnType.equals(Type.getType(Boolean.TYPE))) {
                             Type[] args = Type.getArgumentTypes(m.desc);

src/jdk.jfr/share/classes/jdk/jfr/internal/MetadataLoader.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2021, 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
@@ -255,6 +255,9 @@ private void addFields(Map<String, Type> lookup, Map<String, AnnotationElement>
                 if ("to".equals(f.transition)) {
                     aes.add(TRANSITION_TO);
                 }
+                if (!"package".equals(f.name) && !"java.lang.Class".equals(te.name)) {
+                    Utils.ensureJavaIdentifier(f.name);
+                }
                 type.add(PrivateAccess.getInstance().newValueDescriptor(f.name, fieldType, aes, f.array ? 1 : 0, f.constantPool, null));
             }
         }

src/jdk.jfr/share/classes/jdk/jfr/internal/Type.java

@@ -33,6 +33,7 @@
 import java.util.Map;
 import java.util.Objects;
 
+import jdk.internal.module.Checks;
 import jdk.jfr.AnnotationElement;
 import jdk.jfr.Event;
 import jdk.jfr.SettingControl;
@@ -76,7 +77,7 @@ private static Type createKnownType(Class<?> clazz) {
 
     private static Type createKnownType(String name, Class<?> clazz) {
         long id = JVM.getJVM().getTypeId(name);
-        Type t =  new Type(name, null, id);
+        Type t =  new Type(name, null, id, null);
         knownTypes.put(t, clazz);
         return t;
     }
@@ -99,14 +100,14 @@ private static Type createKnownType(String name, Class<?> clazz) {
      */
     public Type(String javaTypeName, String superType, long typeId) {
         this(javaTypeName, superType, typeId, null);
+        if (!Checks.isClassName(javaTypeName)) {
+            // Should not be able to come here with an invalid type name
+            throw new InternalError(javaTypeName + " is not a valid Java type");
+        }
     }
 
     Type(String javaTypeName, String superType, long typeId, Boolean simpleType) {
         Objects.requireNonNull(javaTypeName);
-
-        if (!isValidJavaIdentifier(javaTypeName)) {
-            throw new IllegalArgumentException(javaTypeName + " is not a valid Java identifier");
-        }
         this.superType = superType;
         this.name = javaTypeName;
         this.id = typeId;
@@ -126,24 +127,6 @@ static Collection<Type> getKnownTypes() {
         return knownTypes.keySet();
     }
 
-    public static boolean isValidJavaIdentifier(String identifier) {
-        if (identifier.isEmpty()) {
-            return false;
-        }
-        if (!Character.isJavaIdentifierStart(identifier.charAt(0))) {
-            return false;
-        }
-        for (int i = 1; i < identifier.length(); i++) {
-            char c = identifier.charAt(i);
-            if (c != '.') {
-                if (!Character.isJavaIdentifierPart(c)) {
-                    return false;
-                }
-            }
-        }
-        return true;
-    }
-
     public static boolean isValidJavaFieldType(String name) {
         for (Map.Entry<Type, Class<?>> entry : knownTypes.entrySet()) {
             Class<?> clazz = entry.getValue();

src/jdk.jfr/share/classes/jdk/jfr/internal/TypeLibrary.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2021, 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
@@ -191,7 +191,10 @@ private static Type getType(Class<?> clazz) {
     private static Type defineType(Class<?> clazz, String superType, boolean eventType) {
         if (!isDefined(clazz)) {
             Name name = clazz.getAnnotation(Name.class);
-            String typeName = name != null ? name.value() : clazz.getName();
+            String typeName = clazz.getName();
+            if (name != null) {
+                typeName = Utils.validTypeName(name.value(), typeName);
+            }
             long id = Type.getTypeId(clazz);
             Type t;
             if (eventType) {
@@ -317,7 +320,6 @@ static void addImplicitFields(Type type, boolean requestable, boolean hasDuratio
             createAnnotationType(Timespan.class);
             createAnnotationType(Timestamp.class);
             createAnnotationType(Label.class);
-            defineType(long.class, null, false);
             implicitFieldTypes = true;
         }
         addFields(type, requestable, hasDuration, hasThread, hasStackTrace, hasCutoff);
@@ -363,7 +365,7 @@ private static ValueDescriptor createField(Field field) {
         Name name = field.getAnnotation(Name.class);
         String useName = fieldName;
         if (name != null) {
-            useName = name.value();
+            useName = Utils.validJavaIdentifier(name.value(), useName);
         }
         List<jdk.jfr.AnnotationElement> ans = new ArrayList<>();
         for (Annotation a : resolveRepeatedAnnotations(field.getAnnotations())) {

src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java

@@ -54,6 +54,7 @@
 import java.util.Map;
 import java.util.Objects;
 
+import jdk.internal.module.Checks;
 import jdk.internal.org.objectweb.asm.ClassReader;
 import jdk.internal.org.objectweb.asm.util.CheckClassAdapter;
 import jdk.internal.platform.Metrics;
@@ -841,4 +842,28 @@ public static Instant epochNanosToInstant(long epochNanos) {
     public static long timeToNanos(Instant timestamp) {
         return timestamp.getEpochSecond() * 1_000_000_000L + timestamp.getNano();
     }
+
+    public static String validTypeName(String typeName, String defaultTypeName) {
+        if (Checks.isClassName(typeName)) {
+            return typeName;
+        } else {
+            Logger.log(LogTag.JFR, LogLevel.WARN, "@Name ignored, not a valid Java type name.");
+            return defaultTypeName;
+        }
+    }
+
+    public static String validJavaIdentifier(String identifier, String defaultIdentifier) {
+        if (Checks.isJavaIdentifier(identifier)) {
+            return identifier;
+        } else {
+            Logger.log(LogTag.JFR, LogLevel.WARN, "@Name ignored, not a valid Java identifier.");
+            return defaultIdentifier;
+        }
+    }
+
+    public static void ensureJavaIdentifier(String name) {
+        if (!Checks.isJavaIdentifier(name)) {
+            throw new IllegalArgumentException("'" + name + "' is not a valid Java identifier");
+        }
+    }
 }