From ae42125b8c14b65c43af7f907f53681750ea7a0f Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 2 May 2022 13:01:56 -0400 Subject: [PATCH 1/2] Implement safety tracking through StringBuilder/StringBuffer This adds support for both fluent and non-fluent cases, where the latter operates as a compound assignment, updating the receiver variable/field if present. --- .../safety/SafetyPropagationTransfer.java | 46 ++++++++++++++++++- .../IllegalSafeLoggingArgumentTest.java | 44 ++++++++++++++++++ 2 files changed, 88 insertions(+), 2 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 70facf13a..b36f3d20c 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 @@ -286,6 +286,16 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction< THROWABLES_STACK_TRACE_AS_STRING, PRIMITIVE_BOXING); + private static final Matcher CONSTRUCTOR_SAFETY_COMBINATION_OF_ARGS = Matchers.anyOf( + MethodMatchers.constructor().forClass(StringBuilder.class.getName()).withParameters(String.class.getName()), + MethodMatchers.constructor() + .forClass(StringBuilder.class.getName()) + .withParameters(CharSequence.class.getName()), + MethodMatchers.constructor().forClass(StringBuffer.class.getName()).withParameters(String.class.getName()), + MethodMatchers.constructor() + .forClass(StringBuffer.class.getName()) + .withParameters(CharSequence.class.getName())); + private static final Matcher OPTIONAL_ACCESSORS = Matchers.anyOf( MethodMatchers.instanceMethod() .onDescendantOf(Optional.class.getName()) @@ -359,6 +369,15 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction< .onClass("com.palantir.logsafe.Preconditions") .namedAnyOf("checkNotNull", "checkArgumentNotNull")); + // Similar to RETURNS_SAFETY_OF_ARGS_AND_RECEIVER, except the variable itself is assigned the safety result. + // For example, the following returns do-not-log due to a mutation on the second line: + // StringBuilder sb = new StringBuilder().append(safe); + // sb.append(doNotLog); + // return sb.append(safe).toString(); + private static final Matcher MUTABLE_BUILDER_METHODS = + Matchers.anyOf(MethodMatchers.instanceMethod() + .onExactClassAny(StringBuilder.class.getName(), StringBuffer.class.getName()) + .namedAnyOf("append", "insert", "replace")); private static final Matcher RETURNS_SAFETY_OF_ARGS_AND_RECEIVER = Matchers.anyOf( MethodMatchers.instanceMethod() .onDescendantOf(Stream.class.getName()) @@ -366,7 +385,8 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction< MethodMatchers.instanceMethod() .onDescendantOf(Optional.class.getName()) // TODO(ckozak): support 'or' and 'orElseGet' which require lambda support - .named("orElse")); + .named("orElse"), + MUTABLE_BUILDER_METHODS); private VisitorState state; private final Set traversed = new HashSet<>(); @@ -1084,7 +1104,22 @@ public TransferResult> visitMethodInvocation( Safety methodSymbolSafety = getMethodSymbolSafety(node, input); Safety knownMethodSafety = getKnownMethodSafety(node, input); Safety result = Safety.mergeAssumingUnknownIsSame(methodSymbolSafety, knownMethodSafety); - return noStoreChanges(result, input); + if (MUTABLE_BUILDER_METHODS.matches(node.getTree(), state)) { + ReadableUpdates updates = new ReadableUpdates(); + Node current = node.getTarget().getReceiver(); + while (current instanceof MethodInvocationNode) { + MethodInvocationNode currentInvocation = (MethodInvocationNode) current; + if (MUTABLE_BUILDER_METHODS.matches(currentInvocation.getTree(), state)) { + current = currentInvocation.getTarget().getReceiver(); + } else { + break; + } + } + updates.trySet(current, result); + return updateRegularStore(result, input, updates); + } else { + return noStoreChanges(result, input); + } } private Safety getKnownMethodSafety( @@ -1127,6 +1162,13 @@ private Safety getMethodSymbolSafety( public TransferResult> visitObjectCreation( ObjectCreationNode node, TransferInput> input) { Safety result = SafetyAnnotations.getSafety(node.getTree(), state); + if (CONSTRUCTOR_SAFETY_COMBINATION_OF_ARGS.matches(node.getTree(), state)) { + Safety safety = Safety.SAFE; + for (Node argument : node.getArguments()) { + safety = safety.leastUpperBound(getValueOfSubNode(input, argument)); + } + result = Safety.mergeAssumingUnknownIsSame(result, safety); + } return noStoreChanges(result, input); } 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 a8e880b42..26763c3d4 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 @@ -1443,6 +1443,50 @@ public void testPrimitiveUnboxing() { .doTest(); } + @Test + public void testStringBuilder() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " void f(@Safe String safe, @Unsafe String unsafe) {", + " fun(new StringBuilder(safe));", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(new StringBuilder(unsafe));", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(new StringBuilder(safe).append(unsafe).toString());", + " StringBuilder sb = new StringBuilder().append(safe);", + " sb.append(safe).append(unsafe);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(sb.append(safe).toString());", + " }", + " private static void fun(@Safe Object value) {}", + "}") + .doTest(); + } + + @Test + public void testStringBuffer() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " void f(@Safe String safe, @Unsafe String unsafe) {", + " fun(new StringBuffer(safe));", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(new StringBuffer(unsafe));", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(new StringBuffer(safe).append(unsafe).toString());", + " StringBuffer sb = new StringBuffer().append(safe);", + " sb.append(safe).append(unsafe);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(sb.append(safe).toString());", + " }", + " private static void fun(@Safe Object value) {}", + "}") + .doTest(); + } + private CompilationTestHelper helper() { return CompilationTestHelper.newInstance(IllegalSafeLoggingArgument.class, getClass()); } From 446f598b8d552d0a9b89f1216d0eda316ec09c63 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Mon, 2 May 2022 17:06:15 +0000 Subject: [PATCH 2/2] Add generated changelog entries --- changelog/@unreleased/pr-2245.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-2245.v2.yml diff --git a/changelog/@unreleased/pr-2245.v2.yml b/changelog/@unreleased/pr-2245.v2.yml new file mode 100644 index 000000000..6df49d79a --- /dev/null +++ b/changelog/@unreleased/pr-2245.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Implement safety tracking through StringBuilder/StringBuffer + links: + - https://github.com/palantir/gradle-baseline/pull/2245