Skip to content

Commit

Permalink
fix #2328 Handle safety flow through instanceof pattern matching
Browse files Browse the repository at this point in the history
  • Loading branch information
@carterkozak
carterkozak committed Jul 25, 2022
1 parent 1efac9c commit 3c3d093
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 4 deletions.
5 changes: 5 additions & 0 deletions baseline-error-prone/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ import net.ltgt.gradle.errorprone.CheckSeverity
apply plugin: 'java-library'
apply plugin: 'com.palantir.external-publish-jar'

javaVersions {
libraryTarget = 11
runtime = 17
}

dependencies {
implementation 'com.google.errorprone:error_prone_core'
// Ensure a new enough version of dataflow-errorprone is available
Expand Down
39 changes: 37 additions & 2 deletions ...rone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -840,10 +840,13 @@ public TransferResult<Safety, AccessPathStore<Safety>> visitLocalVariable(
// 1. catch (SomeThrowable t)
// 2. referencing a captured local variable within a lambda
// 3. referencing a captured local variable within an anonymous class
// 4. Pattern matching (input instanceof String str)

// Cast a wide net for all throwables (covers catch statements)
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 {
// No safety information found, likely a captured reference used within a lambda or anonymous class.
safety = getCapturedLocalVariableSafety(node);
Expand All @@ -852,6 +855,26 @@ 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 Safety getCapturedLocalVariableSafety(LocalVariableNode node) {
Symbol symbol = ASTHelpers.getSymbol(node.getTree());
if (!(symbol instanceof VarSymbol)) {
Expand Down Expand Up @@ -1084,6 +1107,11 @@ public TransferResult<Safety, AccessPathStore<Safety>> visitTypeCast(
/** Handles type changes (widen, narrow, and cast). */
private TransferResult<Safety, AccessPathStore<Safety>> handleTypeConversion(
Tree newType, Node original, TransferInput<Safety, AccessPathStore<Safety>> input) {
return noStoreChanges(getTypeConversionSafety(newType, original, input), input);
}

private Safety getTypeConversionSafety(
Tree newType, Node original, TransferInput<Safety, AccessPathStore<Safety>> input) {
Safety inputSafety = getValueOfSubNode(input, original);
Safety targetSafety = SafetyAnnotations.getSafety(newType, state);
Safety resultSafety;
Expand All @@ -1098,13 +1126,20 @@ private TransferResult<Safety, AccessPathStore<Safety>> handleTypeConversion(
} else {
resultSafety = Safety.mergeAssumingUnknownIsSame(inputSafety, targetSafety);
}
return noStoreChanges(resultSafety, input);
return resultSafety;
}

@Override
public TransferResult<Safety, AccessPathStore<Safety>> visitInstanceOf(
InstanceOfNode node, TransferInput<Safety, AccessPathStore<Safety>> input) {
// types themselves are generally safe, boolean results of type checks are always safe.
LocalVariableNode bindingVariable = node.getBindingVariable();
if (bindingVariable != null) {
Safety safety = getTypeConversionSafety(node.getTree().getType(), node.getOperand(), input);
ReadableUpdates updates = new ReadableUpdates();
updates.set(bindingVariable, safety);
return updateRegularStore(Safety.SAFE, input, updates);
}
// Otherwise types themselves are generally safe, boolean results of type checks are always safe.
return noStoreChanges(Safety.SAFE, input);
}

Expand Down
76 changes: 74 additions & 2 deletions ...-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;

class IllegalSafeLoggingArgumentTest {

Expand Down Expand Up @@ -1594,7 +1595,7 @@ public void testTypeVariablesInConstructor() {

@Test
public void testRecordConstruction() {
helper().setArgs("--release", "15", "--enable-preview")
helper().setArgs("--release", "17")
.addSourceLines(
"Test.java",
"import com.palantir.logsafe.*;",
Expand All @@ -1610,7 +1611,7 @@ public void testRecordConstruction() {

@Test
public void testRecordComponentUsage() {
helper().setArgs("--release", "15", "--enable-preview")
helper().setArgs("--release", "17")
.addSourceLines(
"Test.java",
"import com.palantir.logsafe.*;",
Expand Down Expand Up @@ -1775,6 +1776,77 @@ public void testSubclassWithLenientSafety() {
.doTest();
}

@Test
@Timeout(10) // https://github.com/palantir/gradle-baseline/issues/2328
public void testPatternMatching() {
helper().setArgs("--release", "17")
.addSourceLines(
"Test.java",
"import com.palantir.logsafe.*;",
"class Test {",
" @DoNotLog static class DoNotLogClass {}",
" @Unsafe Object f(Object value) {",
" if (value instanceof Integer i) {",
" return i;",
" } else if (value instanceof Float f) {",
" return f;",
" } else if (value instanceof Double d) {",
" return d;",
" } else if (value instanceof Byte b) {",
" return b;",
" } else if (value instanceof CharSequence cs) {",
" return cs;",
" } else if (value instanceof String str) {",
" return str;",
" } else if (value instanceof Throwable t) {",
" return t;",
" } else if (value instanceof DoNotLogClass dnl) {",
" // BUG: Diagnostic contains: Dangerous return value: result is 'DO_NOT_LOG'",
" return dnl;",
" }",
" return null;",
" }",
"}")
.doTest();
}

@Test
public void testPatternMatchingUnsafeToSafeSubtype() {
helper().setArgs("--release", "17")
.addSourceLines(
"Test.java",
"import com.palantir.logsafe.*;",
"class Test {",
" @Safe static class SafeClass {}",
" @Safe Object f(@Unsafe Object value) {",
" if (value instanceof SafeClass s) {",
" return s;",
" }",
" return null;",
" }",
"}")
.doTest();
}

@Test
public void testPatternMatchingUnsafeToUnknown() {
helper().setArgs("--release", "17")
.addSourceLines(
"Test.java",
"import com.palantir.logsafe.*;",
"class Test {",
" @Safe static class SafeClass {}",
" @Safe Object f(@Unsafe Object value) {",
" if (value instanceof String s) {",
" // BUG: Diagnostic contains: Dangerous return value: result is 'UNSAFE'",
" return s;",
" }",
" return null;",
" }",
"}")
.doTest();
}

private CompilationTestHelper helper() {
return CompilationTestHelper.newInstance(IllegalSafeLoggingArgument.class, getClass());
}
Expand Down

0 comments on commit 3c3d093

Please sign in to comment.