From 792cb9db04e9327604e0b1f978399c1b45f1324d Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 27 Apr 2022 17:48:17 -0400 Subject: [PATCH 1/2] Results of boolean logic are safe This resolves an issue in which we considered the result of a null-check on an exception unsafe, which is not accurate. The fix is slightly more involved than it would have been due to the discovery of another bug: auto-boxing and auto-unboxing is interpreted as method invocations along the lines of `Boolean.valueOf(bool)` and `Boolean.booleanValue()` respectively. I added safety-passthrough for such methods. --- .../safety/SafetyPropagationTransfer.java | 109 +++++++++++++++--- .../IllegalSafeLoggingArgumentTest.java | 51 ++++++++ 2 files changed, 145 insertions(+), 15 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 dfd73e305..70facf13a 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 @@ -207,6 +207,74 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction< .named("getStackTraceAsString") .withParameters(Throwable.class.getName()); + private static final Matcher PRIMITIVE_BOXING = Matchers.anyOf( + MethodMatchers.staticMethod() + .onClass(Boolean.class.getName()) + .named("valueOf") + .withParameters("boolean"), + MethodMatchers.staticMethod() + .onClass(Integer.class.getName()) + .named("valueOf") + .withParameters("int"), + MethodMatchers.staticMethod() + .onClass(Byte.class.getName()) + .named("valueOf") + .withParameters("byte"), + MethodMatchers.staticMethod() + .onClass(Character.class.getName()) + .named("valueOf") + .withParameters("char"), + MethodMatchers.staticMethod() + .onClass(Double.class.getName()) + .named("valueOf") + .withParameters("double"), + MethodMatchers.staticMethod() + .onClass(Float.class.getName()) + .named("valueOf") + .withParameters("float"), + MethodMatchers.staticMethod() + .onClass(Long.class.getName()) + .named("valueOf") + .withParameters("long"), + MethodMatchers.staticMethod() + .onClass(Short.class.getName()) + .named("valueOf") + .withParameters("short")); + + private static final Matcher PRIMITIVE_UNBOXING = Matchers.anyOf( + MethodMatchers.instanceMethod() + .onExactClass(Boolean.class.getName()) + .named("booleanValue") + .withNoParameters(), + MethodMatchers.instanceMethod() + .onExactClass(Integer.class.getName()) + .named("intValue") + .withNoParameters(), + MethodMatchers.instanceMethod() + .onExactClass(Byte.class.getName()) + .named("byteValue") + .withNoParameters(), + MethodMatchers.instanceMethod() + .onExactClass(Character.class.getName()) + .named("charValue") + .withNoParameters(), + MethodMatchers.instanceMethod() + .onExactClass(Double.class.getName()) + .named("doubleValue") + .withNoParameters(), + MethodMatchers.instanceMethod() + .onExactClass(Float.class.getName()) + .named("floatValue") + .withNoParameters(), + MethodMatchers.instanceMethod() + .onExactClass(Long.class.getName()) + .named("longValue") + .withNoParameters(), + MethodMatchers.instanceMethod() + .onExactClass(Short.class.getName()) + .named("shortValue") + .withNoParameters()); + // These methods do not take the receiver (generally a static class) into account, only the inputs. private static final Matcher RETURNS_SAFETY_COMBINATION_OF_ARGS = Matchers.anyOf( STRING_FORMAT, @@ -215,7 +283,8 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction< OPTIONAL_FACTORIES, STATIC_STREAM_FACTORIES, RID_FACTORY, - THROWABLES_STACK_TRACE_AS_STRING); + THROWABLES_STACK_TRACE_AS_STRING, + PRIMITIVE_BOXING); private static final Matcher OPTIONAL_ACCESSORS = Matchers.anyOf( MethodMatchers.instanceMethod() @@ -278,7 +347,8 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction< .withNoParameters(), OPTIONAL_ACCESSORS, STREAM_ACCESSORS, - THROWABLE_GET_MESSAGE); + THROWABLE_GET_MESSAGE, + PRIMITIVE_UNBOXING); private static final Matcher RETURNS_SAFETY_OF_FIRST_ARG = Matchers.anyOf( MethodMatchers.staticMethod().onClass(Objects.class.getName()).named("requireNonNull"), @@ -616,49 +686,57 @@ public TransferResult> visitStringConcatenateAss @Override public TransferResult> visitLessThan( LessThanNode node, TransferInput> input) { - return binary(node, input); + // 'a < b' in source is safe, regardless of 'a' and 'b'. + return noStoreChanges(Safety.SAFE, input); } @Override public TransferResult> visitLessThanOrEqual( LessThanOrEqualNode node, TransferInput> input) { - return binary(node, input); + // 'a <= b' in source is safe, regardless of 'a' and 'b'. + return noStoreChanges(Safety.SAFE, input); } @Override public TransferResult> visitGreaterThan( - GreaterThanNode node, TransferInput> input) { - return binary(node, input); + GreaterThanNode _node, TransferInput> input) { + // 'a > b' in source is safe, regardless of 'a' and 'b'. + return noStoreChanges(Safety.SAFE, input); } @Override public TransferResult> visitGreaterThanOrEqual( - GreaterThanOrEqualNode node, TransferInput> input) { - return binary(node, input); + GreaterThanOrEqualNode _node, TransferInput> input) { + // 'a >= b' in source is safe, regardless of 'a' and 'b'. + return noStoreChanges(Safety.SAFE, input); } @Override public TransferResult> visitEqualTo( - EqualToNode node, TransferInput> input) { - return binary(node, input); + EqualToNode _node, TransferInput> input) { + // 'a == b' in source is safe, regardless of 'a' and 'b'. + return noStoreChanges(Safety.SAFE, input); } @Override public TransferResult> visitNotEqual( - NotEqualNode node, TransferInput> input) { - return binary(node, input); + NotEqualNode _node, TransferInput> input) { + // 'a != b' in source is safe, regardless of 'a' and 'b'. + return noStoreChanges(Safety.SAFE, input); } @Override public TransferResult> visitConditionalAnd( ConditionalAndNode node, TransferInput> input) { - return binary(node, input); + // 'a && b' in source is safe, regardless of 'a' and 'b'. + return noStoreChanges(Safety.SAFE, input); } @Override public TransferResult> visitConditionalOr( ConditionalOrNode node, TransferInput> input) { - return binary(node, input); + // 'a || b' in source is safe, regardless of 'a' and 'b'. + return noStoreChanges(Safety.SAFE, input); } @Override @@ -972,7 +1050,8 @@ private TransferResult> handleTypeConversion( @Override public TransferResult> visitInstanceOf( InstanceOfNode node, TransferInput> input) { - return unknown(input); + // types themselves are generally safe, boolean results of type checks are always safe. + return noStoreChanges(Safety.SAFE, 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 186f8ab69..314b91f3e 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 @@ -1372,6 +1372,57 @@ public void testUnsafeArgumentForSafeParameter_noRecommendation() { .doTest(); } + @Test + public void testBinaryOperationsOnUnsafeInput() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @Safe", + " void f(@Unsafe Throwable input) {", + " fun(input instanceof InterruptedException);", + " fun(input == null);", + " fun(input != null);", + " fun(input == new Object());", + " fun(input != null && input instanceof InterruptedException);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + + @Test + public void testPrimitiveBoxing() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @Safe", + " void f(@Unsafe long input) {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(input);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + + @Test + public void testPrimitiveUnboxing() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @Safe", + " void f(@Unsafe Long input) {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(input);", + " }", + " private static void fun(@Safe long value) {}", + "}") + .doTest(); + } + private CompilationTestHelper helper() { return CompilationTestHelper.newInstance(IllegalSafeLoggingArgument.class, getClass()); } From e287160fe732eb1cc3c6cd407d4f4ac7d157f895 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Wed, 27 Apr 2022 21:52:47 +0000 Subject: [PATCH 2/2] Add generated changelog entries --- changelog/@unreleased/pr-2232.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-2232.v2.yml diff --git a/changelog/@unreleased/pr-2232.v2.yml b/changelog/@unreleased/pr-2232.v2.yml new file mode 100644 index 000000000..1c43a2bb2 --- /dev/null +++ b/changelog/@unreleased/pr-2232.v2.yml @@ -0,0 +1,5 @@ +type: fix +fix: + description: Results of boolean logic are considered safe + links: + - https://github.com/palantir/gradle-baseline/pull/2232