From 8a69372e3c8935e00be51b09f38f674210156bbf Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Thu, 15 Aug 2024 19:29:06 +0100 Subject: [PATCH 1/2] Add test to demonstrate NPE leaking from SNS instrumentation when publish request just contains a phone number --- .../aws-java-sns-1.0/src/test/groovy/SnsClientTest.groovy | 7 +++++++ .../aws-java-sns-2.0/src/test/groovy/SnsClientTest.groovy | 8 ++++++++ 2 files changed, 15 insertions(+) diff --git a/dd-java-agent/instrumentation/aws-java-sns-1.0/src/test/groovy/SnsClientTest.groovy b/dd-java-agent/instrumentation/aws-java-sns-1.0/src/test/groovy/SnsClientTest.groovy index 5a2092261e8..d008f7626f8 100644 --- a/dd-java-agent/instrumentation/aws-java-sns-1.0/src/test/groovy/SnsClientTest.groovy +++ b/dd-java-agent/instrumentation/aws-java-sns-1.0/src/test/groovy/SnsClientTest.groovy @@ -211,7 +211,14 @@ abstract class SnsClientTest extends VersionedNamingTestBase { traceContextInJson['x-datadog-parent-id'] == sendSpan.spanId.toString() traceContextInJson['x-datadog-sampling-priority'] == "1" !traceContextInJson['dd-pathway-ctx-base64'].toString().isBlank() + } + + def "SNS message to phone number doesn't leak exception"() { + when: + snsClient.publish(new PublishRequest().withPhoneNumber("+19995550123").withMessage('sometext')) + then: + noExceptionThrown() } } diff --git a/dd-java-agent/instrumentation/aws-java-sns-2.0/src/test/groovy/SnsClientTest.groovy b/dd-java-agent/instrumentation/aws-java-sns-2.0/src/test/groovy/SnsClientTest.groovy index 4cc6f65f54a..12249bf2d7d 100644 --- a/dd-java-agent/instrumentation/aws-java-sns-2.0/src/test/groovy/SnsClientTest.groovy +++ b/dd-java-agent/instrumentation/aws-java-sns-2.0/src/test/groovy/SnsClientTest.groovy @@ -180,6 +180,14 @@ abstract class SnsClientTest extends VersionedNamingTestBase { traceContextInJson['x-datadog-sampling-priority'] == "1" !traceContextInJson['dd-pathway-ctx-base64'].toString().isBlank() } + + def "SNS message to phone number doesn't leak exception"() { + when: + snsClient.publish { it.message("sometext").phoneNumber("+19995550123") } + + then: + noExceptionThrown() + } } class SnsClientV0Test extends SnsClientTest { From b2cc6e8aac86bb10d5419d67e7d49ab93ce584de Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Thu, 15 Aug 2024 19:37:18 +0100 Subject: [PATCH 2/2] Both topicArn and targetArn are optional for SNS publish requests; avoid NPE when only a phone number is provided --- .../instrumentation/aws/v1/sns/SnsInterceptor.java | 10 ++++++++-- .../instrumentation/aws/v2/sns/SnsInterceptor.java | 9 ++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/dd-java-agent/instrumentation/aws-java-sns-1.0/src/main/java/datadog/trace/instrumentation/aws/v1/sns/SnsInterceptor.java b/dd-java-agent/instrumentation/aws-java-sns-1.0/src/main/java/datadog/trace/instrumentation/aws/v1/sns/SnsInterceptor.java index 1bd5a44d7dc..3f1d6162d17 100644 --- a/dd-java-agent/instrumentation/aws-java-sns-1.0/src/main/java/datadog/trace/instrumentation/aws/v1/sns/SnsInterceptor.java +++ b/dd-java-agent/instrumentation/aws-java-sns-1.0/src/main/java/datadog/trace/instrumentation/aws/v1/sns/SnsInterceptor.java @@ -58,8 +58,14 @@ public AmazonWebServiceRequest beforeMarshalling(AmazonWebServiceRequest request // the limit still applies here if (messageAttributes.size() < 10) { // Extract the topic name from the ARN for DSM - String topicName = - pRequest.getTopicArn() == null ? pRequest.getTargetArn() : pRequest.getTopicArn(); + String topicName = pRequest.getTopicArn(); + if (null == topicName) { + topicName = pRequest.getTargetArn(); + if (null == topicName) { + return request; // request is to phone number, ignore for DSM + } + } + topicName = topicName.substring(topicName.lastIndexOf(':') + 1); HashMap modifiedMessageAttributes = diff --git a/dd-java-agent/instrumentation/aws-java-sns-2.0/src/main/java/datadog/trace/instrumentation/aws/v2/sns/SnsInterceptor.java b/dd-java-agent/instrumentation/aws-java-sns-2.0/src/main/java/datadog/trace/instrumentation/aws/v2/sns/SnsInterceptor.java index 172eb0e3bf6..c3e87028521 100644 --- a/dd-java-agent/instrumentation/aws-java-sns-2.0/src/main/java/datadog/trace/instrumentation/aws/v2/sns/SnsInterceptor.java +++ b/dd-java-agent/instrumentation/aws-java-sns-2.0/src/main/java/datadog/trace/instrumentation/aws/v2/sns/SnsInterceptor.java @@ -59,7 +59,14 @@ public SdkRequest modifyRequest( // the limit still applies here if (request.messageAttributes().size() < 10) { // Get topic name for DSM - String snsTopicArn = request.topicArn() == null ? request.targetArn() : request.topicArn(); + String snsTopicArn = request.topicArn(); + if (null == snsTopicArn) { + snsTopicArn = request.targetArn(); + if (null == snsTopicArn) { + return request; // request is to phone number, ignore for DSM + } + } + String snsTopicName = snsTopicArn.substring(snsTopicArn.lastIndexOf(':') + 1); Map modifiedMessageAttributes = new HashMap<>(request.messageAttributes());