[GR-13355] Disallow direct annotation access.

PullRequest: graal/2765

compiler/src/org.graalvm.compiler.code/src/org/graalvm/compiler/code/CompilationResult.java

@@ -615,7 +615,7 @@ public int getTargetCodeSize() {
     /**
      * @return the code annotations or {@code null} if there are none
      */
-    public List<CodeAnnotation> getAnnotations() {
+    public List<CodeAnnotation> getCodeAnnotations() {
         if (annotations == null) {
             return Collections.emptyList();
         }
@@ -706,7 +706,8 @@ public boolean hasUnsafeAccess() {
      * Clears the information in this object pertaining to generating code. That is, the
      * {@linkplain #getMarks() marks}, {@linkplain #getInfopoints() infopoints},
      * {@linkplain #getExceptionHandlers() exception handlers}, {@linkplain #getDataPatches() data
-     * patches} and {@linkplain #getAnnotations() annotations} recorded in this object are cleared.
+     * patches} and {@linkplain #getCodeAnnotations() annotations} recorded in this object are
+     * cleared.
      */
     public void resetForEmittingCode() {
         checkOpen();

compiler/src/org.graalvm.compiler.code/src/org/graalvm/compiler/code/HexCodeFileDisassemblerProvider.java

@@ -78,7 +78,7 @@ private static String disassemble(CodeCacheProvider codeCache, CompilationResult
         long start = installedCode == null ? 0L : installedCode.getStart();
         HexCodeFile hcf = new HexCodeFile(code, start, target.arch.getName(), target.wordSize * 8);
         if (compResult != null) {
-            HexCodeFile.addAnnotations(hcf, compResult.getAnnotations());
+            HexCodeFile.addAnnotations(hcf, compResult.getCodeAnnotations());
             addExceptionHandlersComment(compResult, hcf);
             Register fp = regConfig.getFrameRegister();
             RefMapFormatter slotFormatter = new DefaultRefMapFormatter(target.wordSize, fp, 0);

compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/HotSpotCompiledCodeBuilder.java

@@ -88,7 +88,7 @@ public static HotSpotCompiledCode createCompiledCode(CodeCacheProvider codeCache
 
         ResolvedJavaMethod[] methods = compResult.getMethods();
 
-        List<CodeAnnotation> annotations = compResult.getAnnotations();
+        List<CodeAnnotation> annotations = compResult.getCodeAnnotations();
         Comment[] comments = new Comment[annotations.size()];
         if (!annotations.isEmpty()) {
             for (int i = 0; i < comments.length; i++) {

compiler/src/org.graalvm.util/src/org/graalvm/util/DirectAnnotationAccess.java

@@ -0,0 +1,47 @@
+/*
+ * Copyright (c) 2019, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package org.graalvm.util;
+
+//Checkstyle: allow reflection
+import java.lang.annotation.Annotation;
+import java.lang.reflect.AnnotatedElement;
+
+/**
+ * Wrapper class for annotation access. The purpose of this class is to encapsulate the
+ * AnnotatedElement.getAnnotation() to avoid the use of the "Checkstyle: allow direct annotation
+ * access " and "Checkstyle: disallow direct annotation access" comments for situations where the
+ * annotation access doesn't need to guarded, i.e., in runtime code or code that accesses annotation
+ * on non-user types. See {@link GuardedAnnotationAccess} for details on these checkstyle rules.
+ */
+public class DirectAnnotationAccess {
+
+    public static <T extends Annotation> boolean isAnnotationPresent(AnnotatedElement element, Class<T> annotationClass) {
+        return element.getAnnotation(annotationClass) != null;
+    }
+
+    public static <T extends Annotation> T getAnnotation(AnnotatedElement element, Class<T> annotationType) {
+        return element.getAnnotation(annotationType);
+    }
+}

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/api/AnnotationAccess.java → compiler/src/org.graalvm.util/src/org/graalvm/util/GuardedAnnotationAccess.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2019, 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
@@ -22,7 +22,7 @@
  * or visit www.oracle.com if you need additional information or have any
  * questions.
  */
-package com.oracle.graal.pointsto.api;
+package org.graalvm.util;
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.AnnotatedElement;
@@ -32,17 +32,20 @@
  * https://bugs.openjdk.java.net/browse/JDK-7183985: when an annotation declares a Class<?> array
  * parameter and one of the referenced classes is not present on the classpath parsing the
  * annotations will result in an ArrayStoreException instead of caching of a
- * TypeNotPresentExceptionProxy. This is a problem in JDK8 but was fixed in JDK11+.
- * 
- * This wrapper class also defends against incomplete class path issues. If the element for which
- * annotations are queried is a JMVCI value, i.e., a HotSpotResolvedJavaField, or
- * HotSpotResolvedJavaMethod, the annotations are read via HotSpotJDKReflection using the
+ * TypeNotPresentExceptionProxy. This is a problem in JDK8 but was fixed in JDK11+. This wrapper
+ * class also defends against incomplete class path issues. If the element for which annotations are
+ * queried is a JMVCI value, i.e., a HotSpotResolvedJavaField, or HotSpotResolvedJavaMethod, the
+ * annotations are read via HotSpotJDKReflection using the
  * getFieldAnnotation()/getMethodAnnotation() methods which first construct the field/method object
  * via CompilerToVM.asReflectionField()/CompilerToVM.asReflectionExecutable() which eagerly try to
  * resolve the types referenced in the element signature. If a field declared type or a method
  * return type is missing then JVMCI throws a NoClassDefFoundError.
  */
-public final class AnnotationAccess {
+public final class GuardedAnnotationAccess {
+
+    public static boolean isAnnotationPresent(AnnotatedElement element, Class<? extends Annotation> annotationClass) {
+        return getAnnotation(element, annotationClass) != null;
+    }
 
     public static <T extends Annotation> T getAnnotation(AnnotatedElement element, Class<T> annotationType) {
         try {

substratevm/src/com.oracle.graal.pointsto/.checkstyle_checks.xml

@@ -11,6 +11,25 @@
 <module name="Checker">
   <property name="severity" value="error"/>
   <module name="TreeWalker">
+
+    <!-- Disallows direct calls to "AnnotatedElement.get(Declared)Annotation(s)". This check can yield false positives,
+    i.e., it will match any ".get(Declared)Annotation(s)" calls that are not preceded by "GuardedAnnotationAccess"
+    since checkstyle relies only on pattern matching and it cannot determine the type of the call's receiver object. -->
+    <module name="RegexpSinglelineJava">
+      <property name="id" value="annotationAccess"/>
+      <metadata name="net.sf.eclipsecs.core.comment" value="Disallow calls to AnnotatedElement.get(Declared)Annotation(s)."/>
+      <property name="severity" value="error"/>
+      <property name="format" value="\b(?&lt;!AnnotationAccess)\.(getAnnotation|getAnnotations|getDeclaredAnnotation|getDeclaredAnnotations|isAnnotationPresent)\b"/>
+      <property name="message" value="Direct calls to java.lang.reflect.AnnotatedElement.get(Declared)Annotation(s) are restricted. Use either org.graalvm.util.GuardedAnnotationAccess or org.graalvm.util.DirectAnnotationAccess methods. (Use &quot;// Checkstyle: allow direct annotation access... // Checkstyle: disallow direct annotation access&quot; to disable this check.)"/>
+      <property name="ignoreComments" value="true"/>
+    </module>
+    <module name="SuppressionCommentFilter">
+      <property name="offCommentFormat" value="Checkstyle: allow direct annotation access"/>
+      <property name="onCommentFormat" value="Checkstyle: disallow direct annotation access"/>
+      <property name="checkFormat" value="annotationAccess"/>
+      <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="Disable annotation access checks"/>
+    </module>
+
     <module name="AvoidStarImport">
       <property name="allowClassImports" value="false"/>
       <property name="allowStaticMemberImports" value="false"/>

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java

@@ -106,6 +106,7 @@
 import org.graalvm.compiler.replacements.nodes.BinaryMathIntrinsicNode;
 import org.graalvm.compiler.replacements.nodes.UnaryMathIntrinsicNode;
 import org.graalvm.compiler.word.WordCastNode;
+import org.graalvm.util.GuardedAnnotationAccess;
 
 import com.oracle.graal.pointsto.BigBang;
 import com.oracle.graal.pointsto.api.PointstoOptions;
@@ -300,7 +301,7 @@ private static void registerForeignCall(BigBang bb, ForeignCallDescriptor foreig
 
     protected void apply() {
         // assert method.getAnnotation(Fold.class) == null : method;
-        if (method.getAnnotation(NodeIntrinsic.class) != null) {
+        if (GuardedAnnotationAccess.isAnnotationPresent(method, NodeIntrinsic.class)) {
             graph.getDebug().log("apply MethodTypeFlow on node intrinsic %s", method);
             AnalysisType returnType = (AnalysisType) method.getSignature().getReturnType(method.getDeclaringClass());
             if (returnType.getJavaKind() == JavaKind.Object) {

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisField.java

@@ -33,7 +33,7 @@
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.atomic.AtomicBoolean;
 
-import com.oracle.graal.pointsto.api.AnnotationAccess;
+import org.graalvm.util.GuardedAnnotationAccess;
 import com.oracle.graal.pointsto.api.DefaultUnsafePartition;
 import com.oracle.graal.pointsto.api.HostVM;
 import com.oracle.graal.pointsto.api.PointstoOptions;
@@ -386,17 +386,17 @@ public boolean isStatic() {
 
     @Override
     public Annotation[] getAnnotations() {
-        return AnnotationAccess.getAnnotations(wrapped);
+        return GuardedAnnotationAccess.getAnnotations(wrapped);
     }
 
     @Override
     public Annotation[] getDeclaredAnnotations() {
-        return AnnotationAccess.getDeclaredAnnotations(wrapped);
+        return GuardedAnnotationAccess.getDeclaredAnnotations(wrapped);
     }
 
     @Override
     public <T extends Annotation> T getAnnotation(Class<T> annotationClass) {
-        return AnnotationAccess.getAnnotation(wrapped, annotationClass);
+        return GuardedAnnotationAccess.getAnnotation(wrapped, annotationClass);
     }
 
     @Override

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisMethod.java

@@ -42,7 +42,7 @@
 import org.graalvm.compiler.nodes.StructuredGraph;
 import org.graalvm.compiler.nodes.graphbuilderconf.InvocationPlugin;
 
-import com.oracle.graal.pointsto.api.AnnotationAccess;
+import org.graalvm.util.GuardedAnnotationAccess;
 import com.oracle.graal.pointsto.api.PointstoOptions;
 import com.oracle.graal.pointsto.constraints.UnsupportedFeatureException;
 import com.oracle.graal.pointsto.flow.InvokeTypeFlow;
@@ -414,17 +414,17 @@ public ConstantPool getConstantPool() {
 
     @Override
     public Annotation[] getAnnotations() {
-        return AnnotationAccess.getAnnotations(wrapped);
+        return GuardedAnnotationAccess.getAnnotations(wrapped);
     }
 
     @Override
     public Annotation[] getDeclaredAnnotations() {
-        return AnnotationAccess.getDeclaredAnnotations(wrapped);
+        return GuardedAnnotationAccess.getDeclaredAnnotations(wrapped);
     }
 
     @Override
     public <T extends Annotation> T getAnnotation(Class<T> annotationClass) {
-        return AnnotationAccess.getAnnotation(wrapped, annotationClass);
+        return GuardedAnnotationAccess.getAnnotation(wrapped, annotationClass);
     }
 
     @Override

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisType.java

@@ -47,7 +47,7 @@
 
 import com.oracle.graal.pointsto.BigBang;
 import com.oracle.graal.pointsto.BigBang.ConstantObjectsProfiler;
-import com.oracle.graal.pointsto.api.AnnotationAccess;
+import org.graalvm.util.GuardedAnnotationAccess;
 import com.oracle.graal.pointsto.api.DefaultUnsafePartition;
 import com.oracle.graal.pointsto.api.PointstoOptions;
 import com.oracle.graal.pointsto.api.UnsafePartitionKind;
@@ -890,17 +890,17 @@ public AnalysisField[] getStaticFields() {
 
     @Override
     public Annotation[] getAnnotations() {
-        return AnnotationAccess.getAnnotations(wrapped);
+        return GuardedAnnotationAccess.getAnnotations(wrapped);
     }
 
     @Override
     public Annotation[] getDeclaredAnnotations() {
-        return AnnotationAccess.getDeclaredAnnotations(wrapped);
+        return GuardedAnnotationAccess.getDeclaredAnnotations(wrapped);
     }
 
     @Override
     public <T extends Annotation> T getAnnotation(Class<T> annotationClass) {
-        return AnnotationAccess.getAnnotation(wrapped, annotationClass);
+        return GuardedAnnotationAccess.getAnnotation(wrapped, annotationClass);
     }
 
     @Override

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisUniverse.java

@@ -43,11 +43,11 @@
 import org.graalvm.compiler.core.common.SuppressFBWarnings;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platforms;
+import org.graalvm.util.GuardedAnnotationAccess;
 import org.graalvm.word.WordBase;
 
 import com.oracle.graal.pointsto.AnalysisPolicy;
 import com.oracle.graal.pointsto.BigBang;
-import com.oracle.graal.pointsto.api.AnnotationAccess;
 import com.oracle.graal.pointsto.api.HostVM;
 import com.oracle.graal.pointsto.constraints.UnsupportedFeatureException;
 import com.oracle.graal.pointsto.infrastructure.SubstitutionProcessor;
@@ -630,7 +630,7 @@ public AnalysisPolicy analysisPolicy() {
     }
 
     public boolean platformSupported(AnnotatedElement element) {
-        Platforms platformsAnnotation = AnnotationAccess.getAnnotation(element, Platforms.class);
+        Platforms platformsAnnotation = GuardedAnnotationAccess.getAnnotation(element, Platforms.class);
         if (platform == null || platformsAnnotation == null) {
             return true;
         }

substratevm/src/com.oracle.svm.core.graal/src/com/oracle/svm/core/graal/meta/InstalledCodeBuilder.java

@@ -321,7 +321,7 @@ private void installOperation() {
 
         // Build an index of PatchingAnnoations
         Map<Integer, NativeImagePatcher> patches = new HashMap<>();
-        for (CodeAnnotation codeAnnotation : compilation.getAnnotations()) {
+        for (CodeAnnotation codeAnnotation : compilation.getCodeAnnotations()) {
             if (codeAnnotation instanceof NativeImagePatcher) {
                 patches.put(codeAnnotation.position, (NativeImagePatcher) codeAnnotation);
             }

substratevm/src/com.oracle.svm.core.graal/src/com/oracle/svm/core/graal/meta/SubstrateReplacements.java

@@ -67,6 +67,7 @@
 import org.graalvm.nativeimage.Feature;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platforms;
+import org.graalvm.util.DirectAnnotationAccess;
 
 import com.oracle.svm.core.config.ConfigurationValues;
 import com.oracle.svm.core.util.VMError;
@@ -215,7 +216,7 @@ protected EncodedGraph lookupEncodedGraph(ResolvedJavaMethod lookupMethod, Resol
     @Platforms(Platform.HOSTED_ONLY.class)
     @Override
     public void registerSnippet(ResolvedJavaMethod method, ResolvedJavaMethod original, Object receiver, boolean trackNodeSourcePosition) {
-        assert method.getAnnotation(Snippet.class) != null : "Snippet must be annotated with @" + Snippet.class.getSimpleName() + " " + method;
+        assert DirectAnnotationAccess.isAnnotationPresent(method, Snippet.class) : "Snippet must be annotated with @" + Snippet.class.getSimpleName() + " " + method;
         assert method.hasBytecodes() : "Snippet must not be abstract or native";
         assert builder.graphs.get(method) == null : "snippet registered twice: " + method.getName();
 

substratevm/src/com.oracle.svm.core/.checkstyle_checks.xml

@@ -11,6 +11,26 @@
 <module name="Checker">
   <property name="severity" value="error"/>
   <module name="TreeWalker">
+
+    <!-- Disallows direct calls to "AnnotatedElement.get(Declared)Annotation(s)". This check can yield false positives,
+    i.e., it will match any ".get(Declared)Annotation(s)" calls that are not preceded by "GuardedAnnotationAccess"
+    since checkstyle relies only on pattern matching and it cannot determine the type of the call's receiver object.
+     -->
+    <module name="RegexpSinglelineJava">
+      <property name="id" value="annotationAccess"/>
+      <metadata name="net.sf.eclipsecs.core.comment" value="Disallow calls to AnnotatedElement.get(Declared)Annotation(s)."/>
+      <property name="severity" value="error"/>
+      <property name="format" value="\b(?&lt;!AnnotationAccess)\.(getAnnotation|getAnnotations|getDeclaredAnnotation|getDeclaredAnnotations|isAnnotationPresent)\b"/>
+      <property name="message" value="Direct calls to java.lang.reflect.AnnotatedElement.get(Declared)Annotation(s) are restricted. Use either org.graalvm.util.GuardedAnnotationAccess or org.graalvm.util.DirectAnnotationAccess methods. (Use &quot;// Checkstyle: allow direct annotation access... // Checkstyle: disallow direct annotation access&quot; to disable this check.)"/>
+      <property name="ignoreComments" value="true"/>
+    </module>
+    <module name="SuppressionCommentFilter">
+      <property name="offCommentFormat" value="Checkstyle: allow direct annotation access"/>
+      <property name="onCommentFormat" value="Checkstyle: disallow direct annotation access"/>
+      <property name="checkFormat" value="annotationAccess"/>
+      <metadata name="com.atlassw.tools.eclipse.checkstyle.comment" value="Disable annotation access checks"/>
+    </module>
+
     <module name="AvoidStarImport">
       <property name="allowClassImports" value="false"/>
       <property name="allowStaticMemberImports" value="false"/>

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/c/function/CEntryPointErrors.java

@@ -33,6 +33,8 @@
 import java.lang.reflect.Field;
 import java.util.Arrays;
 
+import org.graalvm.util.DirectAnnotationAccess;
+
 import com.oracle.svm.core.util.VMError;
 
 // Checkstyle: resume
@@ -112,7 +114,7 @@ static String getDescription(int code) {
                     continue;
                 }
                 int value = field.getInt(null);
-                String description = field.getDeclaredAnnotation(CEntryPointErrors.Description.class).value();
+                String description = DirectAnnotationAccess.getAnnotation(field, CEntryPointErrors.Description.class).value();
                 maxValue = Math.max(value, maxValue);
                 if (maxValue >= array.length) {
                     array = Arrays.copyOf(array, 2 * maxValue);

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/config/ObjectLayout.java

@@ -26,6 +26,7 @@
 
 import org.graalvm.compiler.core.common.NumUtil;
 import org.graalvm.nativeimage.c.constant.CEnum;
+import org.graalvm.util.GuardedAnnotationAccess;
 import org.graalvm.word.WordBase;
 
 import com.oracle.svm.core.SubstrateTargetDescription;
@@ -182,7 +183,7 @@ public static JavaKind getCallSignatureKind(boolean isEntryPoint, ResolvedJavaTy
         if (metaAccess.lookupJavaType(WordBase.class).isAssignableFrom(type)) {
             return target.wordJavaKind;
         }
-        if (isEntryPoint && type.getAnnotation(CEnum.class) != null) {
+        if (isEntryPoint && GuardedAnnotationAccess.isAnnotationPresent(type, CEnum.class)) {
             return JavaKind.Int;
         }
         return type.getJavaKind();

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/AnnotationsEncoding.java

@@ -31,7 +31,7 @@ public final class AnnotationsEncoding {
 
     private static final Annotation[] EMPTY_ANNOTATION_ARRAY = new Annotation[0];
 
-    public static Annotation[] getAnnotations(Object annotationsEncoding) {
+    public static Annotation[] decodeAnnotations(Object annotationsEncoding) {
         if (annotationsEncoding == null) {
             return EMPTY_ANNOTATION_ARRAY;
         } else if (annotationsEncoding instanceof ArrayStoreException) {
@@ -45,7 +45,7 @@ public static Annotation[] getAnnotations(Object annotationsEncoding) {
     }
 
     @SuppressWarnings("unchecked")
-    public static <T extends Annotation> T getAnnotation(Object annotationsEncoding, Class<T> annotationClass) {
+    public static <T extends Annotation> T decodeAnnotation(Object annotationsEncoding, Class<T> annotationClass) {
         Objects.requireNonNull(annotationClass);
 
         if (annotationsEncoding instanceof ArrayStoreException) {