diff --git a/src/java.base/share/classes/jdk/internal/module/Checks.java b/src/java.base/share/classes/jdk/internal/module/Checks.java index 3b7dd137225..7965391f049 100644 --- a/src/java.base/share/classes/jdk/internal/module/Checks.java +++ b/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 @@ public final class Checks { * 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; diff --git a/src/jdk.jfr/share/classes/jdk/jfr/EventFactory.java b/src/jdk.jfr/share/classes/jdk/jfr/EventFactory.java index bbda1be7490..71b08e74baa 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/EventFactory.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/EventFactory.java @@ -33,6 +33,7 @@ import java.util.List; 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 final class EventFactory { 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)) { diff --git a/src/jdk.jfr/share/classes/jdk/jfr/Name.java b/src/jdk.jfr/share/classes/jdk/jfr/Name.java index 7c43911f01c..26d1b84c436 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/Name.java +++ b/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 @@ import java.lang.annotation.Target; /** * Annotation that sets the default name for an element. *

- * 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. + *

+ * If the specified name is invalid, the annotation is ignored. *

* A stable and easy-to-use event name is of the form: *

diff --git a/src/jdk.jfr/share/classes/jdk/jfr/ValueDescriptor.java b/src/jdk.jfr/share/classes/jdk/jfr/ValueDescriptor.java index 3132d2b050e..26e523ed40c 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/ValueDescriptor.java +++ b/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 { * * *

- * 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 final class ValueDescriptor { * * *

- * 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 final class ValueDescriptor { } } 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 diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/EventControl.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/EventControl.java index 3ef0948a8b3..6a89ce02d1e 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/EventControl.java +++ b/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 @@ public final class EventControl { 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)) { diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java index e59458a9cc0..9320bb83229 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java +++ b/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,6 +213,7 @@ public final class EventInstrumentation { Set methodSet = new HashSet<>(); List 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) { @@ -220,6 +221,15 @@ public final class EventInstrumentation { // 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 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); diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/MetadataLoader.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/MetadataLoader.java index 1c5e5ab7af4..ef9c22556f2 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/MetadataLoader.java +++ b/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 @@ public final class MetadataLoader { 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)); } } diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/Type.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/Type.java index ecdb6fa0157..9b6891bcdec 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/Type.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/Type.java @@ -33,6 +33,7 @@ import java.util.List; 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 @@ public class Type implements Comparable { 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 @@ public class Type implements Comparable { */ 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 @@ public class Type implements Comparable { 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> entry : knownTypes.entrySet()) { Class clazz = entry.getValue(); diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/TypeLibrary.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/TypeLibrary.java index b7fd93fcf07..fb16c92f171 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/TypeLibrary.java +++ b/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 @@ public final class TypeLibrary { 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 @@ public final class TypeLibrary { 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 @@ public final class TypeLibrary { Name name = field.getAnnotation(Name.class); String useName = fieldName; if (name != null) { - useName = name.value(); + useName = Utils.validJavaIdentifier(name.value(), useName); } List ans = new ArrayList<>(); for (Annotation a : resolveRepeatedAnnotations(field.getAnnotations())) { diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java index db38d3a8d1c..38179ba891b 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java @@ -54,6 +54,7 @@ import java.util.List; 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 final class Utils { 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"); + } + } } diff --git a/test/jdk/jdk/jfr/api/metadata/annotations/TestName.java b/test/jdk/jdk/jfr/api/metadata/annotations/TestName.java index 3eb150ade81..ee0d38a58e1 100644 --- a/test/jdk/jdk/jfr/api/metadata/annotations/TestName.java +++ b/test/jdk/jdk/jfr/api/metadata/annotations/TestName.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 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 @@ -23,10 +23,13 @@ package jdk.jfr.api.metadata.annotations; +import java.lang.annotation.Annotation; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; +import java.util.List; +import java.util.Map; import jdk.jfr.AnnotationElement; import jdk.jfr.Event; @@ -56,6 +59,41 @@ public class TestName { @interface NamedAnnotation { } + @MetadataDefinition + @Name("for") + @Target({ ElementType.TYPE }) + @Retention(RetentionPolicy.RUNTIME) + @interface ReservedAnnotation { + } + + @MetadataDefinition + @Name("Hello World") + @Target({ ElementType.TYPE }) + @Retention(RetentionPolicy.RUNTIME) + @interface InvalidAnnotation1 { + } + + @MetadataDefinition + @Name("Bad#Name") + @Target({ ElementType.TYPE }) + @Retention(RetentionPolicy.RUNTIME) + @interface InvalidAnnotation2 { + } + + @MetadataDefinition + @Name("com.example.9thing") + @Target({ ElementType.TYPE }) + @Retention(RetentionPolicy.RUNTIME) + @interface InvalidAnnotation3 { + } + + @MetadataDefinition + @Name("") + @Target({ ElementType.TYPE }) + @Retention(RetentionPolicy.RUNTIME) + @interface EmptyAnnotation { + } + @NamedAnnotation @Name("com.oracle.TestEvent") static class NamedEvent extends Event { @@ -69,6 +107,91 @@ public class TestName { } } + @Name("continue") + static class ReservedEventName extends Event { + } + + static class ReservedFieldName extends Event { + @Name("final") + private int field; + } + + static class ReservedSettingName extends Event { + @SettingDefinition + @Name("true") + public boolean setting(SimpleSetting s) { + return true; + } + } + + @Name("Hello World") + static class InvalidEventName1 extends Event { + } + + @Name("Bad#Name") + static class InvalidEventName2 extends Event { + } + + @Name("com.example.9thing") + static class InvalidEventName3 extends Event { + } + + static class InvalidFieldName1 extends Event { + @Name("foo.bar") + int field; + } + + static class InvalidFieldName2 extends Event { + @Name("Hello World") + int field; + } + + static class InvalidFieldName3 extends Event { + @Name("1up") + int field; + } + + static class InvalidSettingName1 extends Event { + @SettingDefinition + @Name("foo.bar") + public boolean setting(SimpleSetting s) { + return true; + } + } + + static class InvalidSettingName2 extends Event { + @SettingDefinition + @Name("Hello World") + public boolean setting(SimpleSetting s) { + return true; + } + } + + static class InvalidSettingName3 extends Event { + @SettingDefinition + @Name("1up") + public boolean setting(SimpleSetting s) { + return true; + } + } + + @Name("") + static class EmptyEventName extends Event { + } + + static class EmptyFieldName extends Event { + @Name("") + private String field; + } + + static class EmptySettingName extends Event { + @SettingDefinition + @Name("") + public boolean setting(SimpleSetting s) { + return true; + } + } + public static void main(String[] args) throws Exception { EventType t = EventType.getEventType(NamedEvent.class); ValueDescriptor testField = t.getField("testField"); @@ -87,13 +210,93 @@ public class TestName { assertAnnotation(testField.getAnnotation(Name.class), "@Name should be persisted on field"); assertAnnotation(a.getAnnotation(Name.class), "@Name should be persisted on annotations"); assertAnnotation(setting.getAnnotation(Name.class), "@Name should be persisted on setting"); + + // Check invalid event name + assertIllegalEventName(ReservedEventName.class); + assertIllegalEventName(InvalidEventName1.class); + assertIllegalEventName(InvalidEventName2.class); + assertIllegalEventName(InvalidEventName3.class); + assertIllegalEventName(EmptyEventName.class); + + // Check invalid field names + assertIllegalFieldName(ReservedFieldName.class); + assertIllegalFieldName(InvalidFieldName1.class); + assertIllegalFieldName(InvalidFieldName2.class); + assertIllegalFieldName(InvalidFieldName3.class); + assertIllegalFieldName(EmptyFieldName.class); + + // Check invalid setting names + assertIllegalSettingName(ReservedSettingName.class); + assertIllegalSettingName(InvalidSettingName1.class); + assertIllegalSettingName(InvalidSettingName1.class); + assertIllegalSettingName(InvalidSettingName1.class); + assertIllegalSettingName(EmptySettingName.class); + + // Check invalid value descriptor names + testIllegalValueDescriptorName("goto"); + testIllegalValueDescriptorName("Hello World"); + testIllegalValueDescriptorName("1up"); + testIllegalValueDescriptorName("foo.bar"); + testIllegalValueDescriptorName(""); + + // Check invalid annotation names + testIllegalAnnotationName(ReservedAnnotation.class); + testIllegalAnnotationName(InvalidAnnotation1.class); + testIllegalAnnotationName(InvalidAnnotation2.class); + testIllegalAnnotationName(InvalidAnnotation3.class); + testIllegalAnnotationName(EmptyAnnotation.class); } - // Can't use assert since the use toString on the object which doesn't work well JFR proxies. - private static void assertAnnotation(Object annotation,String message) throws Exception { - if (annotation == null) { - throw new Exception(message); - } + private static void assertIllegalEventName(Class eventClass) throws Exception { + EventType type = EventType.getEventType(eventClass); + if (!type.getName().equals(eventClass.getName())) { + throw new Exception("Expected default name " + eventClass.getName() + ", not illegal name " + type.getName()); + } + } + + private static void assertIllegalSettingName(Class eventClass) throws Exception { + EventType type = EventType.getEventType(eventClass); + for (SettingDescriptor s : type.getSettingDescriptors()) { + if (s.getName().equals("setting")) { + return; + } + if (!List.of("threshold", "enabled", "stackTrace").contains(s.getName())) { + throw new Exception("Expected default setting name 'setting' for event " + type.getName() + ", not illegal " + s.getName()); + } + } + } + + private static void assertIllegalFieldName(Class eventClass) throws Exception { + EventType type = EventType.getEventType(eventClass); + if (type.getField("field") == null) { + String illegal = type.getFields().get(type.getFields().size() - 1).getName(); + throw new Exception("Expected default field name 'field' for event " + type.getName() + ", not illegal name " + illegal); + } + } + + private static void testIllegalValueDescriptorName(String illegalName) throws Exception { + try { + new ValueDescriptor(int.class, illegalName); + } catch (IllegalArgumentException iae) { + // OK, as expected + return; + } + throw new Exception("ValueDescriptor should not accept invalid field name '" + illegalName + "'"); + } + + private static void testIllegalAnnotationName(Class annotationClass) throws Exception { + AnnotationElement ae = new AnnotationElement(annotationClass, Map.of()); + if (!ae.getTypeName().equals(annotationClass.getName())) { + throw new Exception("AnnotationElement for class " + annotationClass + " not accept invalid type name '" + ae.getTypeName() + "'"); + } + } + + // Can't use assert since the use toString on the object which doesn't work well + // JFR proxies. + private static void assertAnnotation(Object annotation, String message) throws Exception { + if (annotation == null) { + throw new Exception(message); + } } private static SettingDescriptor getSetting(EventType t, String name) {