safe-logging analysis correctly handles out-of-scope parameter refere…

…nces (#2988) safe-logging analysis correctly handles out-of-scope parameter references
palantir · Jan 9, 2025 · 20880cf · 20880cf
1 parent 7cc703c
commit 20880cf
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 22 deletions.
diff --git a/...rone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/...rone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java
@@ -71,6 +71,7 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
+import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.VariableElement;
 import javax.lang.model.type.TypeMirror;
 import org.checkerframework.errorprone.dataflow.analysis.Analysis;
@@ -869,13 +870,16 @@ public TransferResult<Safety, AccessPathStore<Safety>> visitLocalVariable(
             // 4. Pattern matching (input instanceof String str)
 
             // Cast a wide net for all throwables (covers catch statements)
+            Safety treeSafety = SafetyAnnotations.getSafety(node.getTree(), state);
             if (THROWABLE_SUBTYPE.matches(node.getTree(), state)) {
-                safety = Safety.UNSAFE.leastUpperBound(SafetyAnnotations.getSafety(node.getTree(), state));
-            } else if (isPatternBinding(node, state)) {
-                safety = SafetyAnnotations.getSafety(node.getTree(), state);
-            } else {
+                safety = Safety.UNSAFE.leastUpperBound(treeSafety);
+            } else if (isElementKind(node, ElementKind.LOCAL_VARIABLE) && treeSafety == Safety.UNKNOWN) {
                 // No safety information found, likely a captured reference used within a lambda or anonymous class.
+                // If safety can be computed based on the symbol, use that, otherwise evaluate flow which is much
+                // more expensive.
                 safety = getCapturedLocalVariableSafety(node);
+            } else {
+                safety = treeSafety;
             }
             ReadableUpdates updates = new ReadableUpdates();
             updates.set(node, safety);
@@ -884,24 +888,9 @@ public TransferResult<Safety, AccessPathStore<Safety>> visitLocalVariable(
         return noStoreChanges(safety, input);
     }
 
-    private static boolean isPatternBinding(LocalVariableNode node, VisitorState state) {
-        TreePath varPath = TreePath.getPath(state.getPath().getCompilationUnit(), node.getTree());
-        if (varPath == null) {
-            // Synthetic expression
-            return false;
-        }
-        TreePath parentPath = varPath.getParentPath();
-        if (parentPath == null) {
-            return false;
-        }
-        Tree enclosing = parentPath.getLeaf();
-        if (enclosing == null) {
-            return false;
-        }
-        // JCBindingPattern is newer than our compilation target, so we match strings to avoid
-        // complex build-system configurations.
-        String kindString = enclosing.getKind().name();
-        return "BINDING_PATTERN".equals(kindString);
+    private static boolean isElementKind(LocalVariableNode node, ElementKind elementKind) {
+        Symbol symbol = ASTHelpers.getSymbol(node.getTree());
+        return symbol != null && symbol.getKind() == elementKind;
     }
 
     private Safety getCapturedLocalVariableSafety(LocalVariableNode node) {

diff --git a/...-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/...-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java
@@ -2121,6 +2121,21 @@ void testDiamondTypeSafetyInheritance() {
                 .doTest();
     }
 
+    @Test
+    public void testStaticMethodReturningRunnable() {
+        helper().addSourceLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "class Test {",
+                        "  static Runnable f(@Unsafe Object value) {",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'",
+                        "    return () -> fun(value);",
+                        "  }",
+                        "  private static void fun(@Safe Object value) {}",
+                        "}")
+                .doTest();
+    }
+
     private CompilationTestHelper helper() {
         return CompilationTestHelper.newInstance(IllegalSafeLoggingArgument.class, getClass());
     }

diff --git a/changelog/@unreleased/pr-2988.v2.yml b/changelog/@unreleased/pr-2988.v2.yml
@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: safe-logging analysis correctly handles out-of-scope parameter references
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/2988