From 942ad3f31dca1ae721d8768e0d27a22180876ae5 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 28 Mar 2022 14:46:36 -0400 Subject: [PATCH 01/15] extract safety --- .../IllegalSafeLoggingArgument.java | 34 +---------------- .../baseline/errorprone/safety/Safety.java | 38 +++++++++++++++++++ 2 files changed, 39 insertions(+), 33 deletions(-) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java index b0a842b8b..79e4c95d4 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java @@ -26,6 +26,7 @@ import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.method.MethodMatchers; import com.google.errorprone.util.ASTHelpers; +import com.palantir.baseline.errorprone.safety.Safety; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; @@ -158,37 +159,4 @@ private static Safety getSafety(Symbol symbol, VisitorState state) { } return Safety.UNKNOWN; } - - private enum Safety { - UNKNOWN() { - @Override - boolean allowsValueWith(Safety _valueSafety) { - // No constraints when safety isn't specified - return true; - } - }, - DO_NOT_LOG() { - @Override - boolean allowsValueWith(Safety _valueSafety) { - // do-not-log on a parameter isn't meaningful for callers, only for the implementation - return true; - } - }, - UNSAFE() { - @Override - boolean allowsValueWith(Safety valueSafety) { - // We allow safe data to be provided to an unsafe annotated parameter because that's safe, however - // we should separately flag and prompt migration of such UnsafeArgs to SafeArg. - return valueSafety != Safety.DO_NOT_LOG; - } - }, - SAFE() { - @Override - boolean allowsValueWith(Safety valueSafety) { - return valueSafety == Safety.UNKNOWN || valueSafety == Safety.SAFE; - } - }; - - abstract boolean allowsValueWith(Safety valueSafety); - } } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java new file mode 100644 index 000000000..30827b6c9 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java @@ -0,0 +1,38 @@ +/* + * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ + +package com.palantir.baseline.errorprone.safety; + +public enum Safety { + UNKNOWN() { + @Override + public boolean allowsValueWith(Safety _valueSafety) { + // No constraints when safety isn't specified + return true; + } + }, + DO_NOT_LOG() { + @Override + public boolean allowsValueWith(Safety _valueSafety) { + // do-not-log on a parameter isn't meaningful for callers, only for the implementation + return true; + } + }, + UNSAFE() { + @Override + public boolean allowsValueWith(Safety valueSafety) { + // We allow safe data to be provided to an unsafe annotated parameter because that's safe, however + // we should separately flag and prompt migration of such UnsafeArgs to SafeArg. + return valueSafety != Safety.DO_NOT_LOG; + } + }, + SAFE() { + @Override + public boolean allowsValueWith(Safety valueSafety) { + return valueSafety == Safety.UNKNOWN || valueSafety == Safety.SAFE; + } + }; + + public abstract boolean allowsValueWith(Safety valueSafety); +} \ No newline at end of file From ed96f831155c0fea6b2a348e13b9bb912ba09f7a Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 28 Mar 2022 20:35:51 -0400 Subject: [PATCH 02/15] Implement simple data flow Borrowed heavily from error-prone null checks --- .../IllegalSafeLoggingArgument.java | 66 +- .../baseline/errorprone/safety/Safety.java | 55 +- .../errorprone/safety/SafetyAnalysis.java | 44 ++ .../errorprone/safety/SafetyAnnotations.java | 95 +++ .../safety/SafetyPropagationTransfer.java | 720 ++++++++++++++++++ .../IllegalSafeLoggingArgumentTest.java | 52 ++ 6 files changed, 971 insertions(+), 61 deletions(-) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java index 79e4c95d4..aed9c146e 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java @@ -27,14 +27,13 @@ import com.google.errorprone.matchers.method.MethodMatchers; import com.google.errorprone.util.ASTHelpers; import com.palantir.baseline.errorprone.safety.Safety; +import com.palantir.baseline.errorprone.safety.SafetyAnalysis; +import com.palantir.baseline.errorprone.safety.SafetyAnnotations; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.tree.Tree; -import com.sun.source.tree.TypeCastTree; -import com.sun.tools.javac.code.Symbol; +import com.sun.source.util.TreePath; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.VarSymbol; -import com.sun.tools.javac.code.Type; import java.util.List; /** @@ -54,18 +53,12 @@ severity = BugPattern.SeverityLevel.ERROR, summary = "safe-logging annotations must agree between args and method parameters") public final class IllegalSafeLoggingArgument extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { - private static final String SAFE = "com.palantir.logsafe.Safe"; - private static final String UNSAFE = "com.palantir.logsafe.Unsafe"; - private static final String DO_NOT_LOG = "com.palantir.logsafe.DoNotLog"; private static final String UNSAFE_ARG = "com.palantir.logsafe.UnsafeArg"; private static final Matcher SAFE_ARG_OF_METHOD_MATCHER = MethodMatchers.staticMethod() .onClass("com.palantir.logsafe.SafeArg") .named("of"); - private static final Matcher TO_STRING = - MethodMatchers.instanceMethod().anyClass().named("toString").withNoParameters(); - @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { List arguments = tree.getArguments(); @@ -79,7 +72,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState List parameters = methodSymbol.getParameters(); for (int i = 0; i < parameters.size(); i++) { VarSymbol parameter = parameters.get(i); - Safety parameterSafety = getSafety(parameter, state); + Safety parameterSafety = SafetyAnnotations.getSafety(parameter, state); if (parameterSafety == Safety.UNKNOWN) { // Fast path, avoid analysis when the value isn't provided to a safety-aware consumer continue; @@ -88,7 +81,10 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState int limit = methodSymbol.isVarArgs() && i == parameters.size() - 1 ? arguments.size() : i + 1; for (int j = i; j < limit; j++) { ExpressionTree argument = arguments.get(j); - Safety argumentSafety = getSafety(argument, state); + + Safety argumentSafety = SafetyAnalysis.instance(state.context) + .getSafety(state.withPath(new TreePath(state.getPath(), argument))); + if (!parameterSafety.allowsValueWith(argumentSafety)) { // use state.reportMatch to report all failing arguments if multiple are invalid state.reportMatch(buildDescription(argument) @@ -113,50 +109,4 @@ private static SuggestedFix getSuggestedFix(MethodInvocationTree tree, VisitorSt return SuggestedFix.emptyFix(); } - - private static Safety getSafety(ExpressionTree tree, VisitorState state) { - Tree argument = ASTHelpers.stripParentheses(tree); - // Check annotations on the result type - Type resultType = ASTHelpers.getResultType(tree); - if (resultType != null) { - Safety resultTypeSafety = getSafety(resultType.tsym, state); - if (resultTypeSafety != Safety.UNKNOWN) { - return resultTypeSafety; - } - } - // Unwrap type-casts: 'Object value = (Object) unsafeType;' is still unsafe. - if (argument instanceof TypeCastTree) { - TypeCastTree typeCastTree = (TypeCastTree) argument; - return getSafety(typeCastTree.getExpression(), state); - } - // If the argument is a method invocation, check the method for safety annotations - if (argument instanceof MethodInvocationTree) { - MethodInvocationTree argumentInvocation = (MethodInvocationTree) argument; - MethodSymbol methodSymbol = ASTHelpers.getSymbol(argumentInvocation); - if (methodSymbol != null) { - Safety methodSafety = getSafety(methodSymbol, state); - // non-annotated toString inherits type-level safety. - if (methodSafety == Safety.UNKNOWN && TO_STRING.matches(argumentInvocation, state)) { - return getSafety(ASTHelpers.getReceiver(argumentInvocation), state); - } - return methodSafety; - } - } - // Check the argument symbol itself: - Symbol argumentSymbol = ASTHelpers.getSymbol(argument); - return argumentSymbol != null ? getSafety(argumentSymbol, state) : Safety.UNKNOWN; - } - - private static Safety getSafety(Symbol symbol, VisitorState state) { - if (ASTHelpers.hasAnnotation(symbol, DO_NOT_LOG, state)) { - return Safety.DO_NOT_LOG; - } - if (ASTHelpers.hasAnnotation(symbol, UNSAFE, state)) { - return Safety.UNSAFE; - } - if (ASTHelpers.hasAnnotation(symbol, SAFE, state)) { - return Safety.SAFE; - } - return Safety.UNKNOWN; - } } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java index 30827b6c9..9e093ccdd 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java @@ -1,11 +1,30 @@ /* - * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. */ package com.palantir.baseline.errorprone.safety; -public enum Safety { +import org.checkerframework.errorprone.dataflow.analysis.AbstractValue; + +public enum Safety implements AbstractValue { UNKNOWN() { + @Override + public Safety leastUpperBound(Safety other) { + return other == SAFE ? this : other; + } + @Override public boolean allowsValueWith(Safety _valueSafety) { // No constraints when safety isn't specified @@ -13,6 +32,11 @@ public boolean allowsValueWith(Safety _valueSafety) { } }, DO_NOT_LOG() { + @Override + public Safety leastUpperBound(Safety _other) { + return this; + } + @Override public boolean allowsValueWith(Safety _valueSafety) { // do-not-log on a parameter isn't meaningful for callers, only for the implementation @@ -20,6 +44,11 @@ public boolean allowsValueWith(Safety _valueSafety) { } }, UNSAFE() { + @Override + public Safety leastUpperBound(Safety other) { + return other == DO_NOT_LOG ? other : this; + } + @Override public boolean allowsValueWith(Safety valueSafety) { // We allow safe data to be provided to an unsafe annotated parameter because that's safe, however @@ -28,6 +57,11 @@ public boolean allowsValueWith(Safety valueSafety) { } }, SAFE() { + @Override + public Safety leastUpperBound(Safety other) { + return other; + } + @Override public boolean allowsValueWith(Safety valueSafety) { return valueSafety == Safety.UNKNOWN || valueSafety == Safety.SAFE; @@ -35,4 +69,19 @@ public boolean allowsValueWith(Safety valueSafety) { }; public abstract boolean allowsValueWith(Safety valueSafety); -} \ No newline at end of file + + /** + * Merge Safety using {@link Safety#leastUpperBound(AbstractValue)} except that {@link Safety#UNKNOWN} assumes + * no confidence, preferring the other type if data is available. + * For example, casting from {@link Object} to a known-safe type should result in {@link Safety#SAFE}. + */ + public static Safety mergeAssumingUnknownIsSame(Safety one, Safety two) { + if (one == UNKNOWN) { + return two; + } + if (two == UNKNOWN) { + return one; + } + return one.leastUpperBound(two); + } +} diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java new file mode 100644 index 000000000..9bfa74516 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java @@ -0,0 +1,44 @@ +/* + * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone.safety; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.dataflow.DataFlow; +import com.palantir.baseline.errorprone.safety.SafetyPropagationTransfer.ClearVisitorState; +import com.sun.tools.javac.util.Context; + +public final class SafetyAnalysis { + private static final Context.Key SAFETY_ANALYSIS_KEY = new Context.Key<>(); + private final SafetyPropagationTransfer safetyPropagation = new SafetyPropagationTransfer(); + + public static SafetyAnalysis instance(Context context) { + SafetyAnalysis instance = context.get(SAFETY_ANALYSIS_KEY); + if (instance == null) { + instance = new SafetyAnalysis(); + context.put(SAFETY_ANALYSIS_KEY, instance); + } + return instance; + } + + private SafetyAnalysis() {} + + public Safety getSafety(VisitorState state) { + try (ClearVisitorState ignored = safetyPropagation.setVisitorState(state)) { + return DataFlow.expressionDataflow(state.getPath(), state.context, safetyPropagation); + } + } +} diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java new file mode 100644 index 000000000..5de6bd396 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java @@ -0,0 +1,95 @@ +/* + * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone.safety; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.TypeCastTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Type; +import java.util.Optional; + +public final class SafetyAnnotations { + private static final String SAFE = "com.palantir.logsafe.Safe"; + private static final String UNSAFE = "com.palantir.logsafe.Unsafe"; + private static final String DO_NOT_LOG = "com.palantir.logsafe.DoNotLog"; + + private static final Matcher TO_STRING = + MethodMatchers.instanceMethod().anyClass().named("toString").withNoParameters(); + + public static Safety getSafety(ExpressionTree tree, VisitorState state) { + Tree argument = ASTHelpers.stripParentheses(tree); + // Check annotations on the result type + Type resultType = ASTHelpers.getResultType(tree); + if (resultType != null) { + Safety resultTypeSafety = getSafety(resultType.tsym, state); + if (resultTypeSafety != Safety.UNKNOWN) { + return resultTypeSafety; + } + } + // Unwrap type-casts: 'Object value = (Object) unsafeType;' is still unsafe. + if (argument instanceof TypeCastTree) { + TypeCastTree typeCastTree = (TypeCastTree) argument; + return getSafety(typeCastTree.getExpression(), state); + } + // If the argument is a method invocation, check the method for safety annotations + if (argument instanceof MethodInvocationTree) { + Optional maybeResult = getMethodInvocationResultSafety((MethodInvocationTree) argument, state); + if (maybeResult.isPresent()) { + return maybeResult.get(); + } + } + // Check the argument symbol itself: + Symbol argumentSymbol = ASTHelpers.getSymbol(argument); + return argumentSymbol != null ? getSafety(argumentSymbol, state) : Safety.UNKNOWN; + } + + public static Optional getMethodInvocationResultSafety( + MethodInvocationTree argumentInvocation, VisitorState state) { + MethodSymbol methodSymbol = ASTHelpers.getSymbol(argumentInvocation); + if (methodSymbol != null) { + Safety methodSafety = getSafety(methodSymbol, state); + // non-annotated toString inherits type-level safety. + if (methodSafety == Safety.UNKNOWN && TO_STRING.matches(argumentInvocation, state)) { + return Optional.of(getSafety(ASTHelpers.getReceiver(argumentInvocation), state)); + } + return Optional.of(methodSafety); + } + return Optional.empty(); + } + + public static Safety getSafety(Symbol symbol, VisitorState state) { + if (ASTHelpers.hasAnnotation(symbol, DO_NOT_LOG, state)) { + return Safety.DO_NOT_LOG; + } + if (ASTHelpers.hasAnnotation(symbol, UNSAFE, state)) { + return Safety.UNSAFE; + } + if (ASTHelpers.hasAnnotation(symbol, SAFE, state)) { + return Safety.SAFE; + } + return Safety.UNKNOWN; + } + + private SafetyAnnotations() {} +} diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java new file mode 100644 index 000000000..030d3a038 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -0,0 +1,720 @@ +/* + * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone.safety; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.annotations.CheckReturnValue; +import com.google.errorprone.dataflow.AccessPath; +import com.google.errorprone.dataflow.AccessPathStore; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.TypeCastTree; +import com.sun.tools.javac.code.Symbol; +import java.io.Closeable; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import javax.lang.model.element.VariableElement; +import org.checkerframework.errorprone.dataflow.analysis.ForwardTransferFunction; +import org.checkerframework.errorprone.dataflow.analysis.RegularTransferResult; +import org.checkerframework.errorprone.dataflow.analysis.TransferInput; +import org.checkerframework.errorprone.dataflow.analysis.TransferResult; +import org.checkerframework.errorprone.dataflow.cfg.UnderlyingAST; +import org.checkerframework.errorprone.dataflow.cfg.node.ArrayAccessNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ArrayCreationNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ArrayTypeNode; +import org.checkerframework.errorprone.dataflow.cfg.node.AssertionErrorNode; +import org.checkerframework.errorprone.dataflow.cfg.node.AssignmentNode; +import org.checkerframework.errorprone.dataflow.cfg.node.BinaryOperationNode; +import org.checkerframework.errorprone.dataflow.cfg.node.BitwiseAndNode; +import org.checkerframework.errorprone.dataflow.cfg.node.BitwiseComplementNode; +import org.checkerframework.errorprone.dataflow.cfg.node.BitwiseOrNode; +import org.checkerframework.errorprone.dataflow.cfg.node.BitwiseXorNode; +import org.checkerframework.errorprone.dataflow.cfg.node.BooleanLiteralNode; +import org.checkerframework.errorprone.dataflow.cfg.node.CaseNode; +import org.checkerframework.errorprone.dataflow.cfg.node.CharacterLiteralNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ClassDeclarationNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ClassNameNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ConditionalAndNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ConditionalNotNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ConditionalOrNode; +import org.checkerframework.errorprone.dataflow.cfg.node.DoubleLiteralNode; +import org.checkerframework.errorprone.dataflow.cfg.node.EqualToNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ExplicitThisNode; +import org.checkerframework.errorprone.dataflow.cfg.node.FieldAccessNode; +import org.checkerframework.errorprone.dataflow.cfg.node.FloatLiteralNode; +import org.checkerframework.errorprone.dataflow.cfg.node.FloatingDivisionNode; +import org.checkerframework.errorprone.dataflow.cfg.node.FloatingRemainderNode; +import org.checkerframework.errorprone.dataflow.cfg.node.FunctionalInterfaceNode; +import org.checkerframework.errorprone.dataflow.cfg.node.GreaterThanNode; +import org.checkerframework.errorprone.dataflow.cfg.node.GreaterThanOrEqualNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ImplicitThisNode; +import org.checkerframework.errorprone.dataflow.cfg.node.InstanceOfNode; +import org.checkerframework.errorprone.dataflow.cfg.node.IntegerDivisionNode; +import org.checkerframework.errorprone.dataflow.cfg.node.IntegerLiteralNode; +import org.checkerframework.errorprone.dataflow.cfg.node.IntegerRemainderNode; +import org.checkerframework.errorprone.dataflow.cfg.node.LambdaResultExpressionNode; +import org.checkerframework.errorprone.dataflow.cfg.node.LeftShiftNode; +import org.checkerframework.errorprone.dataflow.cfg.node.LessThanNode; +import org.checkerframework.errorprone.dataflow.cfg.node.LessThanOrEqualNode; +import org.checkerframework.errorprone.dataflow.cfg.node.LocalVariableNode; +import org.checkerframework.errorprone.dataflow.cfg.node.LongLiteralNode; +import org.checkerframework.errorprone.dataflow.cfg.node.MarkerNode; +import org.checkerframework.errorprone.dataflow.cfg.node.MethodAccessNode; +import org.checkerframework.errorprone.dataflow.cfg.node.MethodInvocationNode; +import org.checkerframework.errorprone.dataflow.cfg.node.NarrowingConversionNode; +import org.checkerframework.errorprone.dataflow.cfg.node.Node; +import org.checkerframework.errorprone.dataflow.cfg.node.NotEqualNode; +import org.checkerframework.errorprone.dataflow.cfg.node.NullChkNode; +import org.checkerframework.errorprone.dataflow.cfg.node.NullLiteralNode; +import org.checkerframework.errorprone.dataflow.cfg.node.NumericalAdditionNode; +import org.checkerframework.errorprone.dataflow.cfg.node.NumericalMinusNode; +import org.checkerframework.errorprone.dataflow.cfg.node.NumericalMultiplicationNode; +import org.checkerframework.errorprone.dataflow.cfg.node.NumericalPlusNode; +import org.checkerframework.errorprone.dataflow.cfg.node.NumericalSubtractionNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ObjectCreationNode; +import org.checkerframework.errorprone.dataflow.cfg.node.PackageNameNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ParameterizedTypeNode; +import org.checkerframework.errorprone.dataflow.cfg.node.PrimitiveTypeNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ReturnNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ShortLiteralNode; +import org.checkerframework.errorprone.dataflow.cfg.node.SignedRightShiftNode; +import org.checkerframework.errorprone.dataflow.cfg.node.StringConcatenateAssignmentNode; +import org.checkerframework.errorprone.dataflow.cfg.node.StringConcatenateNode; +import org.checkerframework.errorprone.dataflow.cfg.node.StringConversionNode; +import org.checkerframework.errorprone.dataflow.cfg.node.StringLiteralNode; +import org.checkerframework.errorprone.dataflow.cfg.node.SuperNode; +import org.checkerframework.errorprone.dataflow.cfg.node.SynchronizedNode; +import org.checkerframework.errorprone.dataflow.cfg.node.TernaryExpressionNode; +import org.checkerframework.errorprone.dataflow.cfg.node.ThrowNode; +import org.checkerframework.errorprone.dataflow.cfg.node.TypeCastNode; +import org.checkerframework.errorprone.dataflow.cfg.node.UnaryOperationNode; +import org.checkerframework.errorprone.dataflow.cfg.node.UnsignedRightShiftNode; +import org.checkerframework.errorprone.dataflow.cfg.node.VariableDeclarationNode; +import org.checkerframework.errorprone.dataflow.cfg.node.WideningConversionNode; + +public final class SafetyPropagationTransfer implements ForwardTransferFunction> { + + private VisitorState state; + + @Override + public AccessPathStore initialStore(UnderlyingAST _underlyingAst, List parameters) { + if (parameters == null) { + return AccessPathStore.empty(); + } + AccessPathStore.Builder result = AccessPathStore.empty().toBuilder(); + for (LocalVariableNode param : parameters) { + Safety declared = SafetyAnnotations.getSafety((Symbol) param.getElement(), state); + result.setInformation(AccessPath.fromLocalVariable(param), declared); + } + return result.build(); + } + + public ClearVisitorState setVisitorState(VisitorState value) { + this.state = Objects.requireNonNull(value, "VisitorState"); + return new ClearVisitorState(); + } + + public final class ClearVisitorState implements Closeable { + @Override + public void close() { + SafetyPropagationTransfer.this.state = null; + } + } + + private static TransferResult> noStoreChanges( + Safety value, TransferInput> input) { + return new RegularTransferResult<>(value, input.getRegularStore()); + } + + @CheckReturnValue + private static TransferResult> updateRegularStore( + Safety value, TransferInput> input, ReadableUpdates updates) { + ResultingStore newStore = updateStore(input.getRegularStore(), updates); + return new RegularTransferResult<>(value, newStore.store, newStore.storeChanged); + } + + @CheckReturnValue + private static ResultingStore updateStore(AccessPathStore oldStore, ReadableUpdates... updates) { + AccessPathStore.Builder builder = oldStore.toBuilder(); + for (ReadableUpdates update : updates) { + for (Map.Entry entry : update.values.entrySet()) { + builder.setInformation(entry.getKey(), entry.getValue()); + } + } + AccessPathStore newStore = builder.build(); + return new ResultingStore(newStore, !newStore.equals(oldStore)); + } + + @SuppressWarnings("CheckStyle") + private static final class ResultingStore { + final AccessPathStore store; + final boolean storeChanged; + + ResultingStore(AccessPathStore store, boolean storeChanged) { + this.store = store; + this.storeChanged = storeChanged; + } + } + + interface Updates { + void set(LocalVariableNode node, Safety value); + + void set(VariableDeclarationNode node, Safety value); + + void set(FieldAccessNode node, Safety value); + + void set(AccessPath path, Safety value); + + default void trySet(Node node, Safety value) { + if (node instanceof LocalVariableNode) { + set((LocalVariableNode) node, value); + } else if (node instanceof FieldAccessNode) { + set((FieldAccessNode) node, value); + } else if (node instanceof VariableDeclarationNode) { + set((VariableDeclarationNode) node, value); + } + } + } + + @SuppressWarnings("CheckStyle") + private static final class ReadableUpdates implements Updates { + final Map values = new HashMap<>(); + + @Override + public void set(LocalVariableNode node, Safety value) { + values.put(AccessPath.fromLocalVariable(node), Objects.requireNonNull(value)); + } + + @Override + public void set(VariableDeclarationNode node, Safety value) { + values.put(AccessPath.fromVariableDecl(node), Objects.requireNonNull(value)); + } + + @Override + public void set(FieldAccessNode node, Safety value) { + AccessPath path = AccessPath.fromFieldAccess(node); + if (path != null) { + values.put(path, Objects.requireNonNull(value)); + } + } + + @Override + public void set(AccessPath path, Safety value) { + values.put(Objects.requireNonNull(path), Objects.requireNonNull(value)); + } + } + + private TransferResult> literal( + TransferInput> input) { + ReadableUpdates updates = new ReadableUpdates(); + // Compile-time data is guaranteed to be safe. + return updateRegularStore(Safety.SAFE, input, updates); + } + + private TransferResult> unary( + UnaryOperationNode node, TransferInput> input) { + Safety safety = input.getValueOfSubNode(node.getOperand()); + return noStoreChanges(safety, input); + } + + private TransferResult> binary( + BinaryOperationNode node, TransferInput> input) { + Safety safety = input.getValueOfSubNode(node.getLeftOperand()) + .leastUpperBound(input.getValueOfSubNode(node.getRightOperand())); + return noStoreChanges(safety, input); + } + + // Visitor methods + + @Override + public TransferResult> visitShortLiteral( + ShortLiteralNode _node, TransferInput> input) { + return literal(input); + } + + @Override + public TransferResult> visitIntegerLiteral( + IntegerLiteralNode _node, TransferInput> input) { + return literal(input); + } + + @Override + public TransferResult> visitLongLiteral( + LongLiteralNode _node, TransferInput> input) { + return literal(input); + } + + @Override + public TransferResult> visitFloatLiteral( + FloatLiteralNode _node, TransferInput> input) { + return literal(input); + } + + @Override + public TransferResult> visitDoubleLiteral( + DoubleLiteralNode _node, TransferInput> input) { + return literal(input); + } + + @Override + public TransferResult> visitBooleanLiteral( + BooleanLiteralNode _node, TransferInput> input) { + return literal(input); + } + + @Override + public TransferResult> visitCharacterLiteral( + CharacterLiteralNode _node, TransferInput> input) { + return literal(input); + } + + @Override + public TransferResult> visitStringLiteral( + StringLiteralNode _node, TransferInput> input) { + return literal(input); + } + + @Override + public TransferResult> visitNullLiteral( + NullLiteralNode _node, TransferInput> input) { + return literal(input); + } + + @Override + public TransferResult> visitNumericalMinus( + NumericalMinusNode node, TransferInput> input) { + return unary(node, input); + } + + @Override + public TransferResult> visitNumericalPlus( + NumericalPlusNode node, TransferInput> input) { + return unary(node, input); + } + + @Override + public TransferResult> visitBitwiseComplement( + BitwiseComplementNode node, TransferInput> input) { + return unary(node, input); + } + + @Override + public TransferResult> visitNullChk( + NullChkNode node, TransferInput> input) { + ReadableUpdates updates = new ReadableUpdates(); + // null values are safe, and null check boolean results are safe + return updateRegularStore(Safety.SAFE, input, updates); + } + + @Override + public TransferResult> visitStringConcatenate( + StringConcatenateNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitNumericalAddition( + NumericalAdditionNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitNumericalSubtraction( + NumericalSubtractionNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitNumericalMultiplication( + NumericalMultiplicationNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitIntegerDivision( + IntegerDivisionNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitFloatingDivision( + FloatingDivisionNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitIntegerRemainder( + IntegerRemainderNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitFloatingRemainder( + FloatingRemainderNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitLeftShift( + LeftShiftNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitSignedRightShift( + SignedRightShiftNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitUnsignedRightShift( + UnsignedRightShiftNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitBitwiseAnd( + BitwiseAndNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitBitwiseOr( + BitwiseOrNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitBitwiseXor( + BitwiseXorNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitStringConcatenateAssignment( + StringConcatenateAssignmentNode node, TransferInput> input) { + Safety safety = input.getValueOfSubNode(node.getLeftOperand()) + .leastUpperBound(input.getValueOfSubNode(node.getRightOperand())); + return noStoreChanges(safety, input); + } + + @Override + public TransferResult> visitLessThan( + LessThanNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitLessThanOrEqual( + LessThanOrEqualNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitGreaterThan( + GreaterThanNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitGreaterThanOrEqual( + GreaterThanOrEqualNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitEqualTo( + EqualToNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitNotEqual( + NotEqualNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitConditionalAnd( + ConditionalAndNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitConditionalOr( + ConditionalOrNode node, TransferInput> input) { + return binary(node, input); + } + + @Override + public TransferResult> visitConditionalNot( + ConditionalNotNode node, TransferInput> input) { + return unary(node, input); + } + + @Override + public TransferResult> visitTernaryExpression( + TernaryExpressionNode node, TransferInput> input) { + Safety safety = input.getValueOfSubNode(node.getThenOperand()) + .leastUpperBound(input.getValueOfSubNode(node.getElseOperand())); + return noStoreChanges(safety, input); + } + + @Override + public TransferResult> visitAssignment( + AssignmentNode node, TransferInput> input) { + ReadableUpdates updates = new ReadableUpdates(); + Safety safety = input.getValueOfSubNode(node.getExpression()); + Node target = node.getTarget(); + if (target instanceof LocalVariableNode) { + updates.trySet(target, safety); + } + if (target instanceof ArrayAccessNode) { + updates.trySet(((ArrayAccessNode) target).getArray(), safety); + } + if (target instanceof FieldAccessNode) { + FieldAccessNode fieldAccess = (FieldAccessNode) target; + if (!fieldAccess.isStatic()) { + updates.trySet(fieldAccess.getReceiver(), safety); + } + updates.set(fieldAccess, safety); + } + return updateRegularStore(safety, input, updates); + } + + @Override + public TransferResult> visitLocalVariable( + LocalVariableNode node, TransferInput> input) { + ReadableUpdates updates = new ReadableUpdates(); + Safety safety = hasNonNullConstantValue(node) + ? Safety.SAFE + : input.getRegularStore().valueOfAccessPath(AccessPath.fromLocalVariable(node), Safety.UNKNOWN); + return updateRegularStore(safety, input, updates); + } + + private static boolean hasNonNullConstantValue(LocalVariableNode node) { + if (node.getElement() instanceof VariableElement) { + VariableElement element = (VariableElement) node.getElement(); + return (element.getConstantValue() != null); + } + return false; + } + + @Override + public TransferResult> visitVariableDeclaration( + VariableDeclarationNode node, TransferInput> input) { + ReadableUpdates updates = new ReadableUpdates(); + Safety safety = + SafetyAnnotations.getSafety(ASTHelpers.getSymbol(node.getTree().getType()), state); + return updateRegularStore(safety, input, updates); + } + + @Override + public TransferResult> visitFieldAccess( + FieldAccessNode node, TransferInput> input) { + Safety fieldSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(node.getTree()), state); + Safety typeSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(node.getTree()).tsym, state); + Safety safety = Safety.mergeAssumingUnknownIsSame(fieldSafety, typeSafety); + return updateRegularStore(safety, input, new ReadableUpdates()); + } + + @Override + public TransferResult> visitMethodAccess( + MethodAccessNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitArrayAccess( + ArrayAccessNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitImplicitThis( + ImplicitThisNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitExplicitThis( + ExplicitThisNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitSuper( + SuperNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitReturn( + ReturnNode node, TransferInput> input) { + Node result = node.getResult(); + Safety safety = result == null ? Safety.SAFE : input.getValueOfSubNode(result); + return noStoreChanges(safety, input); + } + + @Override + public TransferResult> visitLambdaResultExpression( + LambdaResultExpressionNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitStringConversion( + StringConversionNode node, TransferInput> input) { + Safety safety = input.getValueOfSubNode(node.getOperand()); + return noStoreChanges(safety, input); + } + + @Override + public TransferResult> visitWideningConversion( + WideningConversionNode node, TransferInput> input) { + Safety valueSafety = input.getValueOfSubNode(node.getOperand()); + Safety widenTargetSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(node.getTree()).tsym, state); + Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, widenTargetSafety); + return updateRegularStore(resultSafety, input, new ReadableUpdates()); + } + + @Override + public TransferResult> visitNarrowingConversion( + NarrowingConversionNode node, TransferInput> input) { + Safety valueSafety = input.getValueOfSubNode(node.getOperand()); + Safety narrowTargetSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(node.getTree()).tsym, state); + Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, narrowTargetSafety); + return updateRegularStore(resultSafety, input, new ReadableUpdates()); + } + + @Override + public TransferResult> visitInstanceOf( + InstanceOfNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitTypeCast( + TypeCastNode node, TransferInput> input) { + Safety valueSafety = input.getValueOfSubNode(node.getOperand()); + TypeCastTree castTree = (TypeCastTree) node.getTree(); + Safety castTargetSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(castTree.getType()).tsym, state); + Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, castTargetSafety); + return updateRegularStore(resultSafety, input, new ReadableUpdates()); + } + + @Override + public TransferResult> visitSynchronized( + SynchronizedNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitAssertionError( + AssertionErrorNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitThrow( + ThrowNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitCase( + CaseNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitMethodInvocation( + MethodInvocationNode node, TransferInput> input) { + Safety result = SafetyAnnotations.getMethodInvocationResultSafety(node.getTree(), state) + .orElse(Safety.UNKNOWN); + ReadableUpdates updates = new ReadableUpdates(); + return updateRegularStore(result, input, updates); + } + + @Override + public TransferResult> visitObjectCreation( + ObjectCreationNode node, TransferInput> input) { + Safety result = SafetyAnnotations.getSafety(node.getTree(), state); + ReadableUpdates updates = new ReadableUpdates(); + return updateRegularStore(result, input, updates); + } + + @Override + public TransferResult> visitMemberReference( + FunctionalInterfaceNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitArrayCreation( + ArrayCreationNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitArrayType( + ArrayTypeNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitPrimitiveType( + PrimitiveTypeNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitClassName( + ClassNameNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitPackageName( + PackageNameNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitParameterizedType( + ParameterizedTypeNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitMarker( + MarkerNode node, TransferInput> input) { + return stubUnknown(input); + } + + @Override + public TransferResult> visitClassDeclaration( + ClassDeclarationNode node, TransferInput> input) { + return stubUnknown(input); + } + + private static TransferResult> stubUnknown( + TransferInput> input) { + return new RegularTransferResult<>(Safety.UNKNOWN, input.getRegularStore()); + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java index 914c92f3d..27bc1529d 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java @@ -203,6 +203,58 @@ public void testSafeReturn() { .doTest(); } + @Test + public void testSafeReturnIndirect() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog static Object doNotLogMethod() { return null; }", + " @Safe static Object safeMethod() { return null; }", + " @Unsafe static Object unsafeMethod() { return null; }", + " void f() {", + " Object one = doNotLogMethod();", + " Object two = safeMethod();", + " Object three = unsafeMethod();", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG' " + + "but the parameter requires 'SAFE'.", + " fun(one);", + " fun(two);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' " + + "but the parameter requires 'SAFE'.", + " fun(three);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + + @Test + public void testStringConcatenationWithUnsafe() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog static Object doNotLogMethod() { return null; }", + " @Safe static Object safeMethod() { return null; }", + " @Unsafe static Object unsafeMethod() { return null; }", + " void f() {", + " Object one = doNotLogMethod();", + " Object two = safeMethod();", + " Object three = unsafeMethod();", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG' " + + "but the parameter requires 'SAFE'.", + " fun(\"test\" + one);", + " fun(\"test\" + two);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' " + + "but the parameter requires 'SAFE'.", + " fun(\"test\" + three);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + @Test public void testVarArgReturn() { helper().addSourceLines( From b9c10efe0f322dad254f5d7715ed70a20ac724c0 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 28 Mar 2022 20:49:41 -0400 Subject: [PATCH 03/15] style --- .../errorprone/safety/SafetyAnnotations.java | 26 +++++++++---------- .../safety/SafetyPropagationTransfer.java | 7 +++-- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java index 5de6bd396..47a0bd6b7 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java @@ -64,6 +64,19 @@ public static Safety getSafety(ExpressionTree tree, VisitorState state) { return argumentSymbol != null ? getSafety(argumentSymbol, state) : Safety.UNKNOWN; } + public static Safety getSafety(Symbol symbol, VisitorState state) { + if (ASTHelpers.hasAnnotation(symbol, DO_NOT_LOG, state)) { + return Safety.DO_NOT_LOG; + } + if (ASTHelpers.hasAnnotation(symbol, UNSAFE, state)) { + return Safety.UNSAFE; + } + if (ASTHelpers.hasAnnotation(symbol, SAFE, state)) { + return Safety.SAFE; + } + return Safety.UNKNOWN; + } + public static Optional getMethodInvocationResultSafety( MethodInvocationTree argumentInvocation, VisitorState state) { MethodSymbol methodSymbol = ASTHelpers.getSymbol(argumentInvocation); @@ -78,18 +91,5 @@ public static Optional getMethodInvocationResultSafety( return Optional.empty(); } - public static Safety getSafety(Symbol symbol, VisitorState state) { - if (ASTHelpers.hasAnnotation(symbol, DO_NOT_LOG, state)) { - return Safety.DO_NOT_LOG; - } - if (ASTHelpers.hasAnnotation(symbol, UNSAFE, state)) { - return Safety.UNSAFE; - } - if (ASTHelpers.hasAnnotation(symbol, SAFE, state)) { - return Safety.SAFE; - } - return Safety.UNKNOWN; - } - private SafetyAnnotations() {} } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java index 030d3a038..bed01d719 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -162,8 +162,8 @@ private static ResultingStore updateStore(AccessPathStore oldStore, Read @SuppressWarnings("CheckStyle") private static final class ResultingStore { - final AccessPathStore store; - final boolean storeChanged; + private final AccessPathStore store; + private final boolean storeChanged; ResultingStore(AccessPathStore store, boolean storeChanged) { this.store = store; @@ -191,9 +191,8 @@ default void trySet(Node node, Safety value) { } } - @SuppressWarnings("CheckStyle") private static final class ReadableUpdates implements Updates { - final Map values = new HashMap<>(); + private final Map values = new HashMap<>(); @Override public void set(LocalVariableNode node, Safety value) { From 63387219c752840459e6dcef547056abea8098e3 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 28 Mar 2022 21:05:57 -0400 Subject: [PATCH 04/15] cleaner --- .../safety/SafetyPropagationTransfer.java | 55 ++++++++++--------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java index bed01d719..6d81e4ae7 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -107,6 +107,9 @@ import org.checkerframework.errorprone.dataflow.cfg.node.VariableDeclarationNode; import org.checkerframework.errorprone.dataflow.cfg.node.WideningConversionNode; +/** + * Inspired heavily by error-prone NullnessPropagationTransfer (apache 2). + */ public final class SafetyPropagationTransfer implements ForwardTransferFunction> { private VisitorState state; @@ -218,10 +221,15 @@ public void set(AccessPath path, Safety value) { } } + private static TransferResult> unknown( + TransferInput> input) { + return new RegularTransferResult<>(Safety.UNKNOWN, input.getRegularStore()); + } + private TransferResult> literal( TransferInput> input) { ReadableUpdates updates = new ReadableUpdates(); - // Compile-time data is guaranteed to be safe. + // Compile-time data (literal) is guaranteed to be safe. return updateRegularStore(Safety.SAFE, input, updates); } @@ -535,31 +543,31 @@ public TransferResult> visitFieldAccess( @Override public TransferResult> visitMethodAccess( MethodAccessNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitArrayAccess( ArrayAccessNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitImplicitThis( ImplicitThisNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitExplicitThis( ExplicitThisNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitSuper( SuperNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override @@ -573,7 +581,7 @@ public TransferResult> visitReturn( @Override public TransferResult> visitLambdaResultExpression( LambdaResultExpressionNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override @@ -604,7 +612,7 @@ public TransferResult> visitNarrowingConversion( @Override public TransferResult> visitInstanceOf( InstanceOfNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override @@ -620,25 +628,25 @@ public TransferResult> visitTypeCast( @Override public TransferResult> visitSynchronized( SynchronizedNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitAssertionError( AssertionErrorNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitThrow( ThrowNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitCase( CaseNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override @@ -661,59 +669,54 @@ public TransferResult> visitObjectCreation( @Override public TransferResult> visitMemberReference( FunctionalInterfaceNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitArrayCreation( ArrayCreationNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitArrayType( ArrayTypeNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitPrimitiveType( PrimitiveTypeNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitClassName( ClassNameNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitPackageName( PackageNameNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitParameterizedType( ParameterizedTypeNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitMarker( MarkerNode node, TransferInput> input) { - return stubUnknown(input); + return unknown(input); } @Override public TransferResult> visitClassDeclaration( ClassDeclarationNode node, TransferInput> input) { - return stubUnknown(input); - } - - private static TransferResult> stubUnknown( - TransferInput> input) { - return new RegularTransferResult<>(Safety.UNKNOWN, input.getRegularStore()); + return unknown(input); } } From 620df75cab84d8354a8305382b5a43e3b51b1a5b Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Tue, 29 Mar 2022 01:06:22 +0000 Subject: [PATCH 05/15] Add generated changelog entries --- changelog/@unreleased/pr-2143.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-2143.v2.yml diff --git a/changelog/@unreleased/pr-2143.v2.yml b/changelog/@unreleased/pr-2143.v2.yml new file mode 100644 index 000000000..4256fb916 --- /dev/null +++ b/changelog/@unreleased/pr-2143.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Implement Safety flow checks + links: + - https://github.com/palantir/gradle-baseline/pull/2143 From 9c545182e384c4abb3324f26756283a1f67b3cd4 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 29 Mar 2022 13:24:45 -0400 Subject: [PATCH 06/15] cleaner --- .../baseline/errorprone/safety/Safety.java | 5 + .../errorprone/safety/SafetyAnnotations.java | 76 +++---- .../safety/SafetyPropagationTransfer.java | 187 +++++++++++++++--- .../IllegalSafeLoggingArgumentTest.java | 182 +++++++++++++++++ 4 files changed, 364 insertions(+), 86 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java index 9e093ccdd..82fc6a941 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java @@ -84,4 +84,9 @@ public static Safety mergeAssumingUnknownIsSame(Safety one, Safety two) { } return one.leastUpperBound(two); } + + public static Safety mergeAssumingUnknownIsSame(Safety one, Safety two, Safety three) { + Safety result = mergeAssumingUnknownIsSame(one, two); + return mergeAssumingUnknownIsSame(result, three); + } } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java index 47a0bd6b7..6d59a9740 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java @@ -17,78 +17,46 @@ package com.palantir.baseline.errorprone.safety; import com.google.errorprone.VisitorState; -import com.google.errorprone.matchers.Matcher; -import com.google.errorprone.matchers.method.MethodMatchers; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.tree.Tree; -import com.sun.source.tree.TypeCastTree; import com.sun.tools.javac.code.Symbol; -import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Type; -import java.util.Optional; +import javax.annotation.Nullable; public final class SafetyAnnotations { private static final String SAFE = "com.palantir.logsafe.Safe"; private static final String UNSAFE = "com.palantir.logsafe.Unsafe"; private static final String DO_NOT_LOG = "com.palantir.logsafe.DoNotLog"; - private static final Matcher TO_STRING = - MethodMatchers.instanceMethod().anyClass().named("toString").withNoParameters(); + public static Safety getSafety(ExpressionTree input, VisitorState state) { + // Check the result type + ExpressionTree tree = ASTHelpers.stripParentheses(input); + Safety resultTypeSafet = getResultTypeSafety(tree, state); - public static Safety getSafety(ExpressionTree tree, VisitorState state) { - Tree argument = ASTHelpers.stripParentheses(tree); - // Check annotations on the result type - Type resultType = ASTHelpers.getResultType(tree); - if (resultType != null) { - Safety resultTypeSafety = getSafety(resultType.tsym, state); - if (resultTypeSafety != Safety.UNKNOWN) { - return resultTypeSafety; - } - } - // Unwrap type-casts: 'Object value = (Object) unsafeType;' is still unsafe. - if (argument instanceof TypeCastTree) { - TypeCastTree typeCastTree = (TypeCastTree) argument; - return getSafety(typeCastTree.getExpression(), state); - } - // If the argument is a method invocation, check the method for safety annotations - if (argument instanceof MethodInvocationTree) { - Optional maybeResult = getMethodInvocationResultSafety((MethodInvocationTree) argument, state); - if (maybeResult.isPresent()) { - return maybeResult.get(); - } - } // Check the argument symbol itself: - Symbol argumentSymbol = ASTHelpers.getSymbol(argument); - return argumentSymbol != null ? getSafety(argumentSymbol, state) : Safety.UNKNOWN; + Symbol argumentSymbol = ASTHelpers.getSymbol(tree); + Safety symbolSafety = argumentSymbol != null ? getSafety(argumentSymbol, state) : Safety.UNKNOWN; + return Safety.mergeAssumingUnknownIsSame(resultTypeSafet, symbolSafety); } - public static Safety getSafety(Symbol symbol, VisitorState state) { - if (ASTHelpers.hasAnnotation(symbol, DO_NOT_LOG, state)) { - return Safety.DO_NOT_LOG; - } - if (ASTHelpers.hasAnnotation(symbol, UNSAFE, state)) { - return Safety.UNSAFE; - } - if (ASTHelpers.hasAnnotation(symbol, SAFE, state)) { - return Safety.SAFE; + public static Safety getSafety(@Nullable Symbol symbol, VisitorState state) { + if (symbol != null) { + if (ASTHelpers.hasAnnotation(symbol, DO_NOT_LOG, state)) { + return Safety.DO_NOT_LOG; + } + if (ASTHelpers.hasAnnotation(symbol, UNSAFE, state)) { + return Safety.UNSAFE; + } + if (ASTHelpers.hasAnnotation(symbol, SAFE, state)) { + return Safety.SAFE; + } } return Safety.UNKNOWN; } - public static Optional getMethodInvocationResultSafety( - MethodInvocationTree argumentInvocation, VisitorState state) { - MethodSymbol methodSymbol = ASTHelpers.getSymbol(argumentInvocation); - if (methodSymbol != null) { - Safety methodSafety = getSafety(methodSymbol, state); - // non-annotated toString inherits type-level safety. - if (methodSafety == Safety.UNKNOWN && TO_STRING.matches(argumentInvocation, state)) { - return Optional.of(getSafety(ASTHelpers.getReceiver(argumentInvocation), state)); - } - return Optional.of(methodSafety); - } - return Optional.empty(); + public static Safety getResultTypeSafety(ExpressionTree expression, VisitorState state) { + Type resultType = ASTHelpers.getResultType(expression); + return resultType == null ? Safety.UNKNOWN : getSafety(resultType.tsym, state); } private SafetyAnnotations() {} diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java index 6d81e4ae7..1c7ad3b7c 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -20,20 +20,39 @@ import com.google.errorprone.annotations.CheckReturnValue; import com.google.errorprone.dataflow.AccessPath; import com.google.errorprone.dataflow.AccessPathStore; +import com.google.errorprone.dataflow.AccessPathValues; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.TypeCastTree; +import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreePath; +import com.sun.source.util.Trees; +import com.sun.tools.javac.code.Flags; import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import com.sun.tools.javac.processing.JavacProcessingEnvironment; import java.io.Closeable; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; +import javax.annotation.Nullable; import javax.lang.model.element.VariableElement; +import org.checkerframework.errorprone.dataflow.analysis.Analysis; +import org.checkerframework.errorprone.dataflow.analysis.ForwardAnalysisImpl; import org.checkerframework.errorprone.dataflow.analysis.ForwardTransferFunction; import org.checkerframework.errorprone.dataflow.analysis.RegularTransferResult; import org.checkerframework.errorprone.dataflow.analysis.TransferInput; import org.checkerframework.errorprone.dataflow.analysis.TransferResult; +import org.checkerframework.errorprone.dataflow.cfg.ControlFlowGraph; import org.checkerframework.errorprone.dataflow.cfg.UnderlyingAST; +import org.checkerframework.errorprone.dataflow.cfg.builder.CFGBuilder; import org.checkerframework.errorprone.dataflow.cfg.node.ArrayAccessNode; import org.checkerframework.errorprone.dataflow.cfg.node.ArrayCreationNode; import org.checkerframework.errorprone.dataflow.cfg.node.ArrayTypeNode; @@ -112,7 +131,11 @@ */ public final class SafetyPropagationTransfer implements ForwardTransferFunction> { + private static final Matcher TO_STRING = + MethodMatchers.instanceMethod().anyClass().named("toString").withNoParameters(); + private VisitorState state; + private final Set traversed = new HashSet<>(); @Override public AccessPathStore initialStore(UnderlyingAST _underlyingAst, List parameters) { @@ -129,6 +152,7 @@ public AccessPathStore initialStore(UnderlyingAST _underlyingAst, List> updateRegularStor } @CheckReturnValue - private static ResultingStore updateStore(AccessPathStore oldStore, ReadableUpdates... updates) { + private static ResultingStore updateStore(AccessPathStore oldStore, ReadableUpdates update) { AccessPathStore.Builder builder = oldStore.toBuilder(); - for (ReadableUpdates update : updates) { - for (Map.Entry entry : update.values.entrySet()) { - builder.setInformation(entry.getKey(), entry.getValue()); - } - } + update.values.forEach(builder::setInformation); AccessPathStore newStore = builder.build(); return new ResultingStore(newStore, !newStore.equals(oldStore)); } @@ -192,6 +213,8 @@ default void trySet(Node node, Safety value) { set((VariableDeclarationNode) node, value); } } + + boolean isEmpty(); } private static final class ReadableUpdates implements Updates { @@ -219,6 +242,11 @@ public void set(FieldAccessNode node, Safety value) { public void set(AccessPath path, Safety value) { values.put(Objects.requireNonNull(path), Objects.requireNonNull(value)); } + + @Override + public boolean isEmpty() { + return values.isEmpty(); + } } private static TransferResult> unknown( @@ -228,9 +256,8 @@ private static TransferResult> unknown( private TransferResult> literal( TransferInput> input) { - ReadableUpdates updates = new ReadableUpdates(); // Compile-time data (literal) is guaranteed to be safe. - return updateRegularStore(Safety.SAFE, input, updates); + return noStoreChanges(Safety.SAFE, input); } private TransferResult> unary( @@ -323,9 +350,8 @@ public TransferResult> visitBitwiseComplement( @Override public TransferResult> visitNullChk( NullChkNode node, TransferInput> input) { - ReadableUpdates updates = new ReadableUpdates(); // null values are safe, and null check boolean results are safe - return updateRegularStore(Safety.SAFE, input, updates); + return noStoreChanges(Safety.SAFE, input); } @Override @@ -417,7 +443,9 @@ public TransferResult> visitStringConcatenateAss StringConcatenateAssignmentNode node, TransferInput> input) { Safety safety = input.getValueOfSubNode(node.getLeftOperand()) .leastUpperBound(input.getValueOfSubNode(node.getRightOperand())); - return noStoreChanges(safety, input); + ReadableUpdates updates = new ReadableUpdates(); + updates.trySet(node.getLeftOperand(), safety); + return updateRegularStore(safety, input, updates); } @Override @@ -496,22 +524,96 @@ public TransferResult> visitAssignment( } if (target instanceof FieldAccessNode) { FieldAccessNode fieldAccess = (FieldAccessNode) target; - if (!fieldAccess.isStatic()) { - updates.trySet(fieldAccess.getReceiver(), safety); - } updates.set(fieldAccess, safety); } return updateRegularStore(safety, input, updates); } + @SuppressWarnings("checkstyle:CyclomaticComplexity") + private Safety fieldSafety( + @Nullable VarSymbol accessed, @Nullable AccessPath path, AccessPathValues store) { + if (accessed == null) { + return Safety.UNKNOWN; + } + Safety maybeFlowComputedSafety = (path == null) ? null : store.valueOfAccessPath(path, null); + Safety flowSafety = maybeFlowComputedSafety == null ? Safety.UNKNOWN : maybeFlowComputedSafety; + Safety symbolSafety = SafetyAnnotations.getSafety(accessed, state); + Safety typeSafety = SafetyAnnotations.getSafety(accessed.type.tsym, state); + Safety symbolAndTypeSafety = Safety.mergeAssumingUnknownIsSame(flowSafety, symbolSafety, typeSafety); + if (accessed.getSimpleName().contentEquals("class")) { + // compile-time constant + return Safety.mergeAssumingUnknownIsSame(symbolAndTypeSafety, Safety.SAFE); + } + if (accessed.isEnum()) { + // compile-time enum constant + return Safety.mergeAssumingUnknownIsSame(symbolAndTypeSafety, Safety.SAFE); + } + if (accessed.getConstValue() != null) { + // compile-time constant value + return Safety.mergeAssumingUnknownIsSame(symbolAndTypeSafety, Safety.SAFE); + } + // If a computed value was found, we can avoid expensive initializer interactions + if (maybeFlowComputedSafety == null) { + Safety initializer = fieldInitializerSafetyIfAvailable(accessed); + // If the field is final, we trust the result of the initializer. + // Non-final fields can be "tainted" by initializers (to unsafe/do-not-log) but not + // upgraded to safe. + if ((accessed.flags_field & Flags.FINAL) != 0 + || initializer == Safety.DO_NOT_LOG + || initializer == Safety.UNSAFE) { + return Safety.mergeAssumingUnknownIsSame(symbolAndTypeSafety, initializer); + } + } + return symbolAndTypeSafety; + } + + private Safety fieldInitializerSafetyIfAvailable(VarSymbol accessed) { + if (!traversed.add(accessed)) { + // Initializer circularity + return Safety.UNKNOWN; + } + + try { + JavacProcessingEnvironment javacEnv = JavacProcessingEnvironment.instance(state.context); + TreePath fieldDeclPath = Trees.instance(javacEnv).getPath(accessed); + // Skip initializers in other compilation units as analysis of such nodes can fail due to + // missing types. + if (fieldDeclPath == null + || fieldDeclPath.getCompilationUnit() != state.getPath().getCompilationUnit() + || !(fieldDeclPath.getLeaf() instanceof VariableTree)) { + return Safety.UNKNOWN; + } + + ExpressionTree initializer = ((VariableTree) fieldDeclPath.getLeaf()).getInitializer(); + if (initializer == null) { + return Safety.UNKNOWN; + } + + ClassTree classTree = (ClassTree) fieldDeclPath.getParentPath().getLeaf(); + + // Run flow analysis on field initializer. This is inefficient compared to just walking + // the initializer expression tree but it avoids duplicating the logic from this transfer + // function into a method that operates on Javac Nodes. + TreePath initializerPath = TreePath.getPath(fieldDeclPath, initializer); + UnderlyingAST ast = new UnderlyingAST.CFGStatement(initializerPath.getLeaf(), classTree); + ControlFlowGraph cfg = CFGBuilder.build(initializerPath, ast, false, false, javacEnv); + Analysis, SafetyPropagationTransfer> analysis = + new ForwardAnalysisImpl<>(this); + analysis.performAnalysis(cfg); + Safety maybeResult = analysis.getValue(initializerPath.getLeaf()); + return maybeResult == null ? Safety.UNKNOWN : maybeResult; + } finally { + traversed.remove(accessed); + } + } + @Override public TransferResult> visitLocalVariable( LocalVariableNode node, TransferInput> input) { - ReadableUpdates updates = new ReadableUpdates(); Safety safety = hasNonNullConstantValue(node) ? Safety.SAFE : input.getRegularStore().valueOfAccessPath(AccessPath.fromLocalVariable(node), Safety.UNKNOWN); - return updateRegularStore(safety, input, updates); + return noStoreChanges(safety, input); } private static boolean hasNonNullConstantValue(LocalVariableNode node) { @@ -525,10 +627,9 @@ private static boolean hasNonNullConstantValue(LocalVariableNode node) { @Override public TransferResult> visitVariableDeclaration( VariableDeclarationNode node, TransferInput> input) { - ReadableUpdates updates = new ReadableUpdates(); Safety safety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(node.getTree().getType()), state); - return updateRegularStore(safety, input, updates); + return noStoreChanges(safety, input); } @Override @@ -536,8 +637,11 @@ public TransferResult> visitFieldAccess( FieldAccessNode node, TransferInput> input) { Safety fieldSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(node.getTree()), state); Safety typeSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(node.getTree()).tsym, state); - Safety safety = Safety.mergeAssumingUnknownIsSame(fieldSafety, typeSafety); - return updateRegularStore(safety, input, new ReadableUpdates()); + VarSymbol symbol = (VarSymbol) ASTHelpers.getSymbol(node.getTree()); + AccessPath maybeAccessPath = AccessPath.fromFieldAccess(node); + Safety flowSafety = fieldSafety(symbol, maybeAccessPath, input.getRegularStore()); + Safety safety = Safety.mergeAssumingUnknownIsSame(fieldSafety, typeSafety, flowSafety); + return noStoreChanges(safety, input); } @Override @@ -555,19 +659,25 @@ public TransferResult> visitArrayAccess( @Override public TransferResult> visitImplicitThis( ImplicitThisNode node, TransferInput> input) { - return unknown(input); + Symbol symbol = ASTHelpers.getSymbol(node.getTree()); + Safety safety = symbol == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(symbol.owner, state); + return noStoreChanges(safety, input); } @Override public TransferResult> visitExplicitThis( ExplicitThisNode node, TransferInput> input) { - return unknown(input); + Symbol symbol = ASTHelpers.getSymbol(node.getTree()); + Safety safety = symbol == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(symbol.owner, state); + return noStoreChanges(safety, input); } @Override public TransferResult> visitSuper( SuperNode node, TransferInput> input) { - return unknown(input); + Symbol symbol = ASTHelpers.getSymbol(node.getTree()); + Safety safety = symbol == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(symbol.owner, state); + return noStoreChanges(safety, input); } @Override @@ -597,7 +707,7 @@ public TransferResult> visitWideningConversion( Safety valueSafety = input.getValueOfSubNode(node.getOperand()); Safety widenTargetSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(node.getTree()).tsym, state); Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, widenTargetSafety); - return updateRegularStore(resultSafety, input, new ReadableUpdates()); + return noStoreChanges(resultSafety, input); } @Override @@ -606,7 +716,7 @@ public TransferResult> visitNarrowingConversion( Safety valueSafety = input.getValueOfSubNode(node.getOperand()); Safety narrowTargetSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(node.getTree()).tsym, state); Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, narrowTargetSafety); - return updateRegularStore(resultSafety, input, new ReadableUpdates()); + return noStoreChanges(resultSafety, input); } @Override @@ -622,7 +732,7 @@ public TransferResult> visitTypeCast( TypeCastTree castTree = (TypeCastTree) node.getTree(); Safety castTargetSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(castTree.getType()).tsym, state); Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, castTargetSafety); - return updateRegularStore(resultSafety, input, new ReadableUpdates()); + return noStoreChanges(resultSafety, input); } @Override @@ -652,18 +762,31 @@ public TransferResult> visitCase( @Override public TransferResult> visitMethodInvocation( MethodInvocationNode node, TransferInput> input) { - Safety result = SafetyAnnotations.getMethodInvocationResultSafety(node.getTree(), state) - .orElse(Safety.UNKNOWN); - ReadableUpdates updates = new ReadableUpdates(); - return updateRegularStore(result, input, updates); + Safety result = getMethodSymbolSafety(node, input); + return noStoreChanges(result, input); + } + + private Safety getMethodSymbolSafety( + MethodInvocationNode node, TransferInput> input) { + MethodSymbol methodSymbol = ASTHelpers.getSymbol(node.getTree()); + if (methodSymbol != null) { + Safety methodSafety = Safety.mergeAssumingUnknownIsSame( + SafetyAnnotations.getSafety(methodSymbol, state), + SafetyAnnotations.getResultTypeSafety(node.getTree(), state)); + // non-annotated toString inherits type-level safety. + if (methodSafety == Safety.UNKNOWN && TO_STRING.matches(node.getTree(), state)) { + return input.getValueOfSubNode(node.getTarget().getReceiver()); + } + return methodSafety; + } + return SafetyAnnotations.getResultTypeSafety(node.getTree(), state); } @Override public TransferResult> visitObjectCreation( ObjectCreationNode node, TransferInput> input) { Safety result = SafetyAnnotations.getSafety(node.getTree(), state); - ReadableUpdates updates = new ReadableUpdates(); - return updateRegularStore(result, input, updates); + return noStoreChanges(result, input); } @Override diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java index 27bc1529d..274b3b9ed 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java @@ -319,6 +319,132 @@ public void testTypes_toStringUsesObjectSafety() { .doTest(); } + @Test + public void testUnsafeFinalFieldInitializer() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog static Object doNotLogMethod() { return null; }", + " @Safe static Object safeMethod() { return null; }", + " @Unsafe static Object unsafeMethod() { return null; }", + " public final Object one = doNotLogMethod();", + " public final Object two = safeMethod();", + " public final Object three = unsafeMethod();", + " void f() {", + " fun(two);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(three);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG'", + " fun(one);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + + @Test + public void testUnsafeFieldInitializer() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog static Object doNotLogMethod() { return null; }", + " @Safe static Object safeMethod() { return null; }", + " @Unsafe static Object unsafeMethod() { return null; }", + " public Object one = doNotLogMethod();", + " public Object two = safeMethod();", + " public Object three = unsafeMethod();", + " void f() {", + " fun(two);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(three);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG'", + " fun(one);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + + @Test + public void testUnsafeFieldType() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog static class DoNotLogClass {}", + " @Safe static class SafeClass {}", + " @Unsafe static class UnsafeClass {}", + " public DoNotLogClass one;", + " public SafeClass two;", + " public UnsafeClass three;", + " void f() {", + " fun(two);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(three);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG'", + " fun(one);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + + @Test + public void testUnsafeMethodReturnType_retainsUnsafeType() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog static class DoNotLogClass {}", + " @Safe static class SafeClass {}", + " @Unsafe static class UnsafeClass {}", + " static DoNotLogClass doNotLogMethod() { return new DoNotLogClass(); }", + " static SafeClass safeMethod() { return new SafeClass(); }", + " static UnsafeClass unsafeMethod() { return new UnsafeClass(); }", + " void f() {", + " DoNotLogClass one = doNotLogMethod();", + " SafeClass two = safeMethod();", + " UnsafeClass three = unsafeMethod();", + " fun(two);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(three);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG'", + " fun(one);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + + @Test + public void testUnsafeMethodReturnType_widenedType() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog static class DoNotLogClass {}", + " @Safe static class SafeClass {}", + " @Unsafe static class UnsafeClass {}", + " static DoNotLogClass doNotLogMethod() { return new DoNotLogClass(); }", + " static SafeClass safeMethod() { return new SafeClass(); }", + " static UnsafeClass unsafeMethod() { return new UnsafeClass(); }", + " void f() {", + " Object one = doNotLogMethod();", + " Object two = safeMethod();", + " Object three = unsafeMethod();", + " fun(two);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(three);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG'", + " fun(one);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + @Test public void testIncomingParameter_toStringUsesObjectSafety() { helper().addSourceLines( @@ -340,6 +466,62 @@ public void testIncomingParameter_toStringUsesObjectSafety() { .doTest(); } + @Test + public void testStringConcatAssignmentSafety() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " void f(", + " @Safe String safeParam,", + " @Unsafe String unsafeParam,", + " @DoNotLog String doNotLogParam) {", + " String foo = safeParam;", + " fun(foo);", + " foo += unsafeParam;", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(foo);", + " foo += doNotLogParam;", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG'", + " fun(foo);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + + @Test + public void testSafeOfThisUnsafe() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "@Unsafe", + "class Test {", + " void f() {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(this);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + + @Test + public void testSafeOfSuperToStringUnsafe() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "@Unsafe", + "class Test {", + " void f() {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(super.toString());", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + @Test public void testSafeArgOfUnsafe_recommendsUnsafeArgOf() { refactoringHelper() From 09ccf6892b757f3608f9f02de04d2300f22de107 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 29 Mar 2022 13:57:07 -0400 Subject: [PATCH 07/15] simplify --- .../baseline/errorprone/safety/SafetyPropagationTransfer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java index 1c7ad3b7c..7ef2918fe 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -251,7 +251,7 @@ public boolean isEmpty() { private static TransferResult> unknown( TransferInput> input) { - return new RegularTransferResult<>(Safety.UNKNOWN, input.getRegularStore()); + return noStoreChanges(Safety.UNKNOWN, input); } private TransferResult> literal( From da30ac5a15a7cf7a9263b7d61c670acfc7d075ed Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 29 Mar 2022 15:15:02 -0400 Subject: [PATCH 08/15] test --- .../IllegalSafeLoggingArgumentTest.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java index 274b3b9ed..4861620c4 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java @@ -490,6 +490,30 @@ public void testStringConcatAssignmentSafety() { .doTest(); } + @Test + public void testIntegerCompound() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " void f(", + " @Safe int safeParam,", + " @Unsafe int unsafeParam,", + " @DoNotLog int doNotLogParam) {", + " int foo = safeParam;", + " fun(foo);", + " foo += unsafeParam;", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(foo);", + " foo += doNotLogParam;", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG'", + " fun(foo);", + " }", + " private static void fun(@Safe int obj) {}", + "}") + .doTest(); + } + @Test public void testSafeOfThisUnsafe() { helper().addSourceLines( From 174e2db124c4761b0998d67159af4530b25fe477 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 29 Mar 2022 15:39:01 -0400 Subject: [PATCH 09/15] validate return statements --- .../IllegalSafeLoggingArgument.java | 40 +++++++++++++++++-- .../baseline/errorprone/safety/Safety.java | 4 ++ .../IllegalSafeLoggingArgumentTest.java | 26 ++++++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java index aed9c146e..d1a1ee873 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java @@ -31,6 +31,9 @@ import com.palantir.baseline.errorprone.safety.SafetyAnnotations; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.ReturnTree; +import com.sun.source.tree.StatementTree; import com.sun.source.util.TreePath; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.VarSymbol; @@ -52,7 +55,8 @@ linkType = BugPattern.LinkType.CUSTOM, severity = BugPattern.SeverityLevel.ERROR, summary = "safe-logging annotations must agree between args and method parameters") -public final class IllegalSafeLoggingArgument extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { +public final class IllegalSafeLoggingArgument extends BugChecker + implements BugChecker.MethodInvocationTreeMatcher, BugChecker.ReturnTreeMatcher { private static final String UNSAFE_ARG = "com.palantir.logsafe.UnsafeArg"; private static final Matcher SAFE_ARG_OF_METHOD_MATCHER = MethodMatchers.staticMethod() @@ -73,8 +77,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState for (int i = 0; i < parameters.size(); i++) { VarSymbol parameter = parameters.get(i); Safety parameterSafety = SafetyAnnotations.getSafety(parameter, state); - if (parameterSafety == Safety.UNKNOWN) { - // Fast path, avoid analysis when the value isn't provided to a safety-aware consumer + if (parameterSafety.allowsAll()) { + // Fast path: all types are accepted, there's no reason to do further analysis. continue; } @@ -99,6 +103,36 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } + @Override + public Description matchReturn(ReturnTree tree, VisitorState state) { + if (tree.getExpression() == null) { + return Description.NO_MATCH; + } + TreePath path = state.getPath(); + while (path != null && path.getLeaf() instanceof StatementTree) { + path = path.getParentPath(); + } + if (path == null || !(path.getLeaf() instanceof MethodTree)) { + return Description.NO_MATCH; + } + MethodTree method = (MethodTree) path.getLeaf(); + Safety methodDeclaredSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(method), state); + if (methodDeclaredSafety.allowsAll()) { + // Fast path, all types are accepted, there's no reason to do further analysis. + return Description.NO_MATCH; + } + Safety returnValueSafety = SafetyAnalysis.instance(state.context) + .getSafety(state.withPath(new TreePath(state.getPath(), tree.getExpression()))); + if (methodDeclaredSafety.allowsValueWith(returnValueSafety)) { + return Description.NO_MATCH; + } + return buildDescription(tree) + .setMessage(String.format( + "Dangerous return value: result is '%s' but the method is annotated '%s'.", + returnValueSafety, methodDeclaredSafety)) + .build(); + } + private static SuggestedFix getSuggestedFix(MethodInvocationTree tree, VisitorState state, Safety argumentSafety) { if (SAFE_ARG_OF_METHOD_MATCHER.matches(tree, state) && Safety.UNSAFE.allowsValueWith(argumentSafety)) { SuggestedFix.Builder fix = SuggestedFix.builder(); diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java index 82fc6a941..ca59147de 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/Safety.java @@ -70,6 +70,10 @@ public boolean allowsValueWith(Safety valueSafety) { public abstract boolean allowsValueWith(Safety valueSafety); + public boolean allowsAll() { + return this == UNKNOWN || this == DO_NOT_LOG; + } + /** * Merge Safety using {@link Safety#leastUpperBound(AbstractValue)} except that {@link Safety#UNKNOWN} assumes * no confidence, preferring the other type if data is available. diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java index 4861620c4..1b1e6ec20 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java @@ -546,6 +546,32 @@ public void testSafeOfSuperToStringUnsafe() { .doTest(); } + @Test + public void testInvalidReturnValue() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog static class DoNotLogClass {}", + " @Safe static class SafeClass {}", + " @Unsafe static class UnsafeClass {}", + " @Safe Object safe() {", + " return new SafeClass();", + " }", + " @Safe Object unsafe() {", + " // BUG: Diagnostic contains: Dangerous return value: result is 'UNSAFE' " + + "but the method is annotated 'SAFE'.", + " return new UnsafeClass();", + " }", + " @Safe Object doNotLog() {", + " // BUG: Diagnostic contains: Dangerous return value: result is 'DO_NOT_LOG' " + + "but the method is annotated 'SAFE'.", + " return new DoNotLogClass();", + " }", + "}") + .doTest(); + } + @Test public void testSafeArgOfUnsafe_recommendsUnsafeArgOf() { refactoringHelper() From 37cd8e31193f16dd55859f845e4606eb34b70b9a Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 29 Mar 2022 15:45:29 -0400 Subject: [PATCH 10/15] simpler safety-analysis api --- .../IllegalSafeLoggingArgument.java | 7 ++--- .../errorprone/safety/SafetyAnalysis.java | 29 +++++++++++-------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java index d1a1ee873..ec9da9268 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java @@ -86,8 +86,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState for (int j = i; j < limit; j++) { ExpressionTree argument = arguments.get(j); - Safety argumentSafety = SafetyAnalysis.instance(state.context) - .getSafety(state.withPath(new TreePath(state.getPath(), argument))); + Safety argumentSafety = SafetyAnalysis.of(state.withPath(new TreePath(state.getPath(), argument))); if (!parameterSafety.allowsValueWith(argumentSafety)) { // use state.reportMatch to report all failing arguments if multiple are invalid @@ -121,8 +120,8 @@ public Description matchReturn(ReturnTree tree, VisitorState state) { // Fast path, all types are accepted, there's no reason to do further analysis. return Description.NO_MATCH; } - Safety returnValueSafety = SafetyAnalysis.instance(state.context) - .getSafety(state.withPath(new TreePath(state.getPath(), tree.getExpression()))); + Safety returnValueSafety = + SafetyAnalysis.of(state.withPath(new TreePath(state.getPath(), tree.getExpression()))); if (methodDeclaredSafety.allowsValueWith(returnValueSafety)) { return Description.NO_MATCH; } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java index 9bfa74516..a5ab716ce 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnalysis.java @@ -19,26 +19,31 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.dataflow.DataFlow; import com.palantir.baseline.errorprone.safety.SafetyPropagationTransfer.ClearVisitorState; +import com.sun.source.util.TreePath; import com.sun.tools.javac.util.Context; public final class SafetyAnalysis { - private static final Context.Key SAFETY_ANALYSIS_KEY = new Context.Key<>(); - private final SafetyPropagationTransfer safetyPropagation = new SafetyPropagationTransfer(); + private static final Context.Key SAFETY_PROPAGATION = new Context.Key<>(); - public static SafetyAnalysis instance(Context context) { - SafetyAnalysis instance = context.get(SAFETY_ANALYSIS_KEY); + /** + * Returns the safety of the item at the current path. + * Callers may need to use {@link VisitorState#withPath(TreePath)} to provide a more specific path. + */ + public static Safety of(VisitorState state) { + SafetyPropagationTransfer propagation = instance(state.context); + try (ClearVisitorState ignored = propagation.setVisitorState(state)) { + return DataFlow.expressionDataflow(state.getPath(), state.context, propagation); + } + } + + private static SafetyPropagationTransfer instance(Context context) { + SafetyPropagationTransfer instance = context.get(SAFETY_PROPAGATION); if (instance == null) { - instance = new SafetyAnalysis(); - context.put(SAFETY_ANALYSIS_KEY, instance); + instance = new SafetyPropagationTransfer(); + context.put(SAFETY_PROPAGATION, instance); } return instance; } private SafetyAnalysis() {} - - public Safety getSafety(VisitorState state) { - try (ClearVisitorState ignored = safetyPropagation.setVisitorState(state)) { - return DataFlow.expressionDataflow(state.getPath(), state.context, safetyPropagation); - } - } } From 37e6aa08c30eaca3640d0e23b608cfb69fd96872 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 29 Mar 2022 16:34:30 -0400 Subject: [PATCH 11/15] handle polymorphism --- .../errorprone/safety/SafetyAnnotations.java | 46 ++++++++++++++ .../IllegalSafeLoggingArgumentTest.java | 63 +++++++++++++++++++ 2 files changed, 109 insertions(+) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java index 6d59a9740..b02529fba 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java @@ -20,7 +20,11 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.util.List; +import java.util.Objects; import javax.annotation.Nullable; public final class SafetyAnnotations { @@ -39,6 +43,7 @@ public static Safety getSafety(ExpressionTree input, VisitorState state) { return Safety.mergeAssumingUnknownIsSame(resultTypeSafet, symbolSafety); } + @SuppressWarnings("checkstyle:CyclomaticComplexity") public static Safety getSafety(@Nullable Symbol symbol, VisitorState state) { if (symbol != null) { if (ASTHelpers.hasAnnotation(symbol, DO_NOT_LOG, state)) { @@ -50,10 +55,51 @@ public static Safety getSafety(@Nullable Symbol symbol, VisitorState state) { if (ASTHelpers.hasAnnotation(symbol, SAFE, state)) { return Safety.SAFE; } + // Check super-methods + if (symbol instanceof MethodSymbol) { + return getSuperMethodSafety((MethodSymbol) symbol, state); + } + if (symbol instanceof VarSymbol) { + VarSymbol varSymbol = (VarSymbol) symbol; + return getSuperMethodParameterSafety(varSymbol, state); + } } return Safety.UNKNOWN; } + private static Safety getSuperMethodSafety(MethodSymbol method, VisitorState state) { + Safety safety = Safety.UNKNOWN; + if (!method.isStaticOrInstanceInit()) { + for (MethodSymbol superMethod : ASTHelpers.findSuperMethods(method, state.getTypes())) { + safety = Safety.mergeAssumingUnknownIsSame(safety, getSafety(superMethod, state)); + } + } + return safety; + } + + private static Safety getSuperMethodParameterSafety(VarSymbol varSymbol, VisitorState state) { + Safety safety = Safety.UNKNOWN; + if (varSymbol.owner instanceof MethodSymbol) { + // If the owner is a MethodSymbol, this variable is a method parameter + MethodSymbol method = (MethodSymbol) varSymbol.owner; + if (!method.isStaticOrInstanceInit()) { + List methodParameters = method.getParameters(); + for (int i = 0; i < methodParameters.size(); i++) { + VarSymbol current = methodParameters.get(i); + if (Objects.equals(current, varSymbol)) { + for (MethodSymbol superMethod : ASTHelpers.findSuperMethods(method, state.getTypes())) { + safety = Safety.mergeAssumingUnknownIsSame( + safety, + getSafety(superMethod.getParameters().get(i), state)); + } + return safety; + } + } + } + } + return safety; + } + public static Safety getResultTypeSafety(ExpressionTree expression, VisitorState state) { Type resultType = ASTHelpers.getResultType(expression); return resultType == null ? Safety.UNKNOWN : getSafety(resultType.tsym, state); diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java index 1b1e6ec20..91d91a3d8 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java @@ -572,6 +572,69 @@ public void testInvalidReturnValue() { .doTest(); } + @Test + public void testInvalidReturnValue_AnnotatedSupertype() { + helper().addSourceLines( + "Annotated.java", + "import com.palantir.logsafe.*;", + "interface Annotated {", + " @Safe Object safe();", + " @Safe Object unsafe();", + " @Safe Object doNotLog();", + "}") + .addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test implements Annotated {", + " @DoNotLog static class DoNotLogClass {}", + " @Safe static class SafeClass {}", + " @Unsafe static class UnsafeClass {}", + " @Override public Object safe() {", + " return new SafeClass();", + " }", + " @Override public Object unsafe() {", + " // BUG: Diagnostic contains: Dangerous return value: result is 'UNSAFE' " + + "but the method is annotated 'SAFE'.", + " return new UnsafeClass();", + " }", + " @Override public Object doNotLog() {", + " // BUG: Diagnostic contains: Dangerous return value: result is 'DO_NOT_LOG' " + + "but the method is annotated 'SAFE'.", + " return new DoNotLogClass();", + " }", + "}") + .doTest(); + } + + @Test + public void testInvalidValue_AnnotatedSupertype() { + helper().addSourceLines( + "Annotated.java", + "import com.palantir.logsafe.*;", + "interface Annotated {", + " void safe(@Safe Object value);", + "}") + .addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test implements Annotated {", + " @DoNotLog static class DoNotLogClass {}", + " @Safe static class SafeClass {}", + " @Unsafe static class UnsafeClass {}", + " void f() {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG' " + + "but the parameter requires 'SAFE'.", + " safe(new DoNotLogClass());", + " safe(new SafeClass());", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' " + + "but the parameter requires 'SAFE'.", + " safe(new UnsafeClass());", + " }", + " @Override public void safe(Object value) {}", + "}") + .doTest(); + } + @Test public void testSafeArgOfUnsafe_recommendsUnsafeArgOf() { refactoringHelper() From 3ebe69704f667230cede78e24348e1f77599647b Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 29 Mar 2022 16:45:45 -0400 Subject: [PATCH 12/15] nulls --- .../safety/SafetyPropagationTransfer.java | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java index 7ef2918fe..dc07ea8dc 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -34,6 +34,7 @@ import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.VarSymbol; +import com.sun.tools.javac.code.Type; import com.sun.tools.javac.processing.JavacProcessingEnvironment; import java.io.Closeable; import java.util.HashMap; @@ -636,7 +637,8 @@ public TransferResult> visitVariableDeclaration( public TransferResult> visitFieldAccess( FieldAccessNode node, TransferInput> input) { Safety fieldSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(node.getTree()), state); - Safety typeSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(node.getTree()).tsym, state); + Type fieldType = ASTHelpers.getType(node.getTree()); + Safety typeSafety = fieldType == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(fieldType.tsym, state); VarSymbol symbol = (VarSymbol) ASTHelpers.getSymbol(node.getTree()); AccessPath maybeAccessPath = AccessPath.fromFieldAccess(node); Safety flowSafety = fieldSafety(symbol, maybeAccessPath, input.getRegularStore()); @@ -705,7 +707,9 @@ public TransferResult> visitStringConversion( public TransferResult> visitWideningConversion( WideningConversionNode node, TransferInput> input) { Safety valueSafety = input.getValueOfSubNode(node.getOperand()); - Safety widenTargetSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(node.getTree()).tsym, state); + Type widenTarget = ASTHelpers.getType(node.getTree()); + Safety widenTargetSafety = + widenTarget == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(widenTarget.tsym, state); Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, widenTargetSafety); return noStoreChanges(resultSafety, input); } @@ -714,7 +718,9 @@ public TransferResult> visitWideningConversion( public TransferResult> visitNarrowingConversion( NarrowingConversionNode node, TransferInput> input) { Safety valueSafety = input.getValueOfSubNode(node.getOperand()); - Safety narrowTargetSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(node.getTree()).tsym, state); + Type narrowTarget = ASTHelpers.getType(node.getTree()); + Safety narrowTargetSafety = + narrowTarget == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(narrowTarget.tsym, state); Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, narrowTargetSafety); return noStoreChanges(resultSafety, input); } @@ -730,7 +736,9 @@ public TransferResult> visitTypeCast( TypeCastNode node, TransferInput> input) { Safety valueSafety = input.getValueOfSubNode(node.getOperand()); TypeCastTree castTree = (TypeCastTree) node.getTree(); - Safety castTargetSafety = SafetyAnnotations.getSafety(ASTHelpers.getType(castTree.getType()).tsym, state); + Type castTarget = ASTHelpers.getType(castTree.getType()); + Safety castTargetSafety = + castTarget == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(castTarget.tsym, state); Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, castTargetSafety); return noStoreChanges(resultSafety, input); } From 16c4e3b8bc61fce7341716dfb2cc2e4fa180c2cc Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 29 Mar 2022 16:55:15 -0400 Subject: [PATCH 13/15] link upstream --- .../baseline/errorprone/safety/SafetyPropagationTransfer.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java index dc07ea8dc..3ba01ee61 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -128,7 +128,8 @@ import org.checkerframework.errorprone.dataflow.cfg.node.WideningConversionNode; /** - * Inspired heavily by error-prone NullnessPropagationTransfer (apache 2). + * Heavily modified fork from error-prone NullnessPropagationTransfer (apache 2). + * @see NullnessPropagationTransfer */ public final class SafetyPropagationTransfer implements ForwardTransferFunction> { From ae9d3754ee07d58617afbdd882b94f29eb64a964 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 29 Mar 2022 17:01:05 -0400 Subject: [PATCH 14/15] remove unnecessary suppression --- .../palantir/baseline/errorprone/safety/SafetyAnnotations.java | 1 - 1 file changed, 1 deletion(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java index b02529fba..014cbda50 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java @@ -43,7 +43,6 @@ public static Safety getSafety(ExpressionTree input, VisitorState state) { return Safety.mergeAssumingUnknownIsSame(resultTypeSafet, symbolSafety); } - @SuppressWarnings("checkstyle:CyclomaticComplexity") public static Safety getSafety(@Nullable Symbol symbol, VisitorState state) { if (symbol != null) { if (ASTHelpers.hasAnnotation(symbol, DO_NOT_LOG, state)) { From 472f54ded69cf0d4d18b86af72f900a5e925faae Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 30 Mar 2022 12:55:40 -0400 Subject: [PATCH 15/15] review feedback --- .../safety/SafetyPropagationTransfer.java | 127 +++++++++--------- 1 file changed, 63 insertions(+), 64 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java index 3ba01ee61..c6ecd5880 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -26,6 +26,7 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.Tree; import com.sun.source.tree.TypeCastTree; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; @@ -520,17 +521,56 @@ public TransferResult> visitAssignment( Node target = node.getTarget(); if (target instanceof LocalVariableNode) { updates.trySet(target, safety); - } - if (target instanceof ArrayAccessNode) { + } else if (target instanceof ArrayAccessNode) { updates.trySet(((ArrayAccessNode) target).getArray(), safety); - } - if (target instanceof FieldAccessNode) { + } else if (target instanceof FieldAccessNode) { FieldAccessNode fieldAccess = (FieldAccessNode) target; updates.set(fieldAccess, safety); + } else { + throw new UnsupportedOperationException( + "Safety analysis bug, unknown target type: " + target.getClass() + " with value: " + target); } return updateRegularStore(safety, input, updates); } + @Override + public TransferResult> visitLocalVariable( + LocalVariableNode node, TransferInput> input) { + Safety safety = hasNonNullConstantValue(node) + ? Safety.SAFE + : input.getRegularStore().valueOfAccessPath(AccessPath.fromLocalVariable(node), Safety.UNKNOWN); + return noStoreChanges(safety, input); + } + + private static boolean hasNonNullConstantValue(LocalVariableNode node) { + if (node.getElement() instanceof VariableElement) { + VariableElement element = (VariableElement) node.getElement(); + return (element.getConstantValue() != null); + } + return false; + } + + @Override + public TransferResult> visitVariableDeclaration( + VariableDeclarationNode node, TransferInput> input) { + Safety safety = + SafetyAnnotations.getSafety(ASTHelpers.getSymbol(node.getTree().getType()), state); + return noStoreChanges(safety, input); + } + + @Override + public TransferResult> visitFieldAccess( + FieldAccessNode node, TransferInput> input) { + Safety fieldSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(node.getTree()), state); + Type fieldType = ASTHelpers.getType(node.getTree()); + Safety typeSafety = fieldType == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(fieldType.tsym, state); + VarSymbol symbol = (VarSymbol) ASTHelpers.getSymbol(node.getTree()); + AccessPath maybeAccessPath = AccessPath.fromFieldAccess(node); + Safety flowSafety = fieldSafety(symbol, maybeAccessPath, input.getRegularStore()); + Safety safety = Safety.mergeAssumingUnknownIsSame(fieldSafety, typeSafety, flowSafety); + return noStoreChanges(safety, input); + } + @SuppressWarnings("checkstyle:CyclomaticComplexity") private Safety fieldSafety( @Nullable VarSymbol accessed, @Nullable AccessPath path, AccessPathValues store) { @@ -571,7 +611,8 @@ private Safety fieldSafety( private Safety fieldInitializerSafetyIfAvailable(VarSymbol accessed) { if (!traversed.add(accessed)) { - // Initializer circularity + // Avoid infinite recursion between initializers with circular references. We recommend against + // writing such initializers, but handle it gracefully. return Safety.UNKNOWN; } @@ -609,44 +650,6 @@ private Safety fieldInitializerSafetyIfAvailable(VarSymbol accessed) { } } - @Override - public TransferResult> visitLocalVariable( - LocalVariableNode node, TransferInput> input) { - Safety safety = hasNonNullConstantValue(node) - ? Safety.SAFE - : input.getRegularStore().valueOfAccessPath(AccessPath.fromLocalVariable(node), Safety.UNKNOWN); - return noStoreChanges(safety, input); - } - - private static boolean hasNonNullConstantValue(LocalVariableNode node) { - if (node.getElement() instanceof VariableElement) { - VariableElement element = (VariableElement) node.getElement(); - return (element.getConstantValue() != null); - } - return false; - } - - @Override - public TransferResult> visitVariableDeclaration( - VariableDeclarationNode node, TransferInput> input) { - Safety safety = - SafetyAnnotations.getSafety(ASTHelpers.getSymbol(node.getTree().getType()), state); - return noStoreChanges(safety, input); - } - - @Override - public TransferResult> visitFieldAccess( - FieldAccessNode node, TransferInput> input) { - Safety fieldSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(node.getTree()), state); - Type fieldType = ASTHelpers.getType(node.getTree()); - Safety typeSafety = fieldType == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(fieldType.tsym, state); - VarSymbol symbol = (VarSymbol) ASTHelpers.getSymbol(node.getTree()); - AccessPath maybeAccessPath = AccessPath.fromFieldAccess(node); - Safety flowSafety = fieldSafety(symbol, maybeAccessPath, input.getRegularStore()); - Safety safety = Safety.mergeAssumingUnknownIsSame(fieldSafety, typeSafety, flowSafety); - return noStoreChanges(safety, input); - } - @Override public TransferResult> visitMethodAccess( MethodAccessNode node, TransferInput> input) { @@ -707,21 +710,29 @@ public TransferResult> visitStringConversion( @Override public TransferResult> visitWideningConversion( WideningConversionNode node, TransferInput> input) { - Safety valueSafety = input.getValueOfSubNode(node.getOperand()); - Type widenTarget = ASTHelpers.getType(node.getTree()); - Safety widenTargetSafety = - widenTarget == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(widenTarget.tsym, state); - Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, widenTargetSafety); - return noStoreChanges(resultSafety, input); + return handleTypeConversion(node.getTree(), node.getOperand(), input); } @Override public TransferResult> visitNarrowingConversion( NarrowingConversionNode node, TransferInput> input) { - Safety valueSafety = input.getValueOfSubNode(node.getOperand()); - Type narrowTarget = ASTHelpers.getType(node.getTree()); + return handleTypeConversion(node.getTree(), node.getOperand(), input); + } + + @Override + public TransferResult> visitTypeCast( + TypeCastNode node, TransferInput> input) { + TypeCastTree castTree = (TypeCastTree) node.getTree(); + return handleTypeConversion(castTree.getType(), node.getOperand(), input); + } + + /** Handles type changes (widen, narrow, and cast). */ + private TransferResult> handleTypeConversion( + Tree newType, Node original, TransferInput> input) { + Safety valueSafety = input.getValueOfSubNode(original); + Type targetType = ASTHelpers.getType(newType); Safety narrowTargetSafety = - narrowTarget == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(narrowTarget.tsym, state); + targetType == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(targetType.tsym, state); Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, narrowTargetSafety); return noStoreChanges(resultSafety, input); } @@ -732,18 +743,6 @@ public TransferResult> visitInstanceOf( return unknown(input); } - @Override - public TransferResult> visitTypeCast( - TypeCastNode node, TransferInput> input) { - Safety valueSafety = input.getValueOfSubNode(node.getOperand()); - TypeCastTree castTree = (TypeCastTree) node.getTree(); - Type castTarget = ASTHelpers.getType(castTree.getType()); - Safety castTargetSafety = - castTarget == null ? Safety.UNKNOWN : SafetyAnnotations.getSafety(castTarget.tsym, state); - Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, castTargetSafety); - return noStoreChanges(resultSafety, input); - } - @Override public TransferResult> visitSynchronized( SynchronizedNode node, TransferInput> input) {