Skip to content

Fix: Rabbitmq7 header injection overwrites user supplied basic properties #6730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fix: Rabbitmq7 header injection overwrites user supplied basic properties #6730

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fac3d5
Fix: Rabbitmq7 header injection overwrites user supplied basic proper…
Feb 28, 2025
adbc0d4
Fixed CachedBasicPropertiesHelper Activator
Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions ...src/Datadog.Trace/ClrProfiler/AutoInstrumentation/RabbitMQ/CachedBasicPropertiesHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,43 @@
// <copyright file="CachedBasicPropertiesHelper.cs" company="Datadog">
// <copyright file="CachedBasicPropertiesHelper.cs" company="Datadog">
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
// </copyright>

#nullable enable
using System;
using System.Reflection.Emit;
using System.Runtime.InteropServices;
using Datadog.Trace.DuckTyping;
using Datadog.Trace.Util;

namespace Datadog.Trace.ClrProfiler.AutoInstrumentation.RabbitMQ;

internal static class CachedBasicPropertiesHelper<TBasicProperties>
{
// ReSharper disable once StaticMemberInGenericType
private static readonly ActivatorHelper Activator;
private static readonly Func<object, object> Activator;

static CachedBasicPropertiesHelper()
{
Activator = new ActivatorHelper(typeof(TBasicProperties).Assembly.GetType("RabbitMQ.Client.BasicProperties")!);
var targetType = typeof(TBasicProperties).Assembly.GetType("RabbitMQ.Client.BasicProperties")!;
var parameterType = typeof(TBasicProperties).Assembly.GetType("RabbitMQ.Client.IReadOnlyBasicProperties")!;

var constructor = targetType.GetConstructor([parameterType])!;

var createBasicPropertiesMethod = new DynamicMethod(
$"TypeActivator_{targetType.Name}_{parameterType.Name}",
targetType,
[parameterType],
typeof(DuckType).Module,
true);

var il = createBasicPropertiesMethod.GetILGenerator();
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Newobj, constructor);
il.Emit(OpCodes.Ret);
Activator = (Func<object, object>)createBasicPropertiesMethod.CreateDelegate(typeof(Func<object, object>), targetType);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it looks like this is throwing an exception when attempting to run the tests / sample

[ERR] Exception occurred when calling the CallTarget integration continuation. System.TypeInitializationException: The type initializer for 'Datadog.Trace.ClrProfiler.AutoInstrumentation.RabbitMQ.CachedBasicPropertiesHelper`1' threw an exception.
 ---> System.ArgumentException: Cannot bind to the target method because its signature is not compatible with that of the delegate type.
   at System.Delegate.CreateDelegateNoSecurityCheck(Type type, Object target, RuntimeMethodHandle method)
   at System.Reflection.Emit.DynamicMethod.CreateDelegate(Type delegateType, Object target)
   at Datadog.Trace.ClrProfiler.AutoInstrumentation.RabbitMQ.CachedBasicPropertiesHelper`1..cctor()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johang88: After some debugging, we figured out that the call to CreateDelegate() here was throwing an exception about the method signatures not matching. I fixed the issue by removing the targetType parameter, since the dynamic method will not be bound to a type (i.e. it acts like a static method, not an instance method).

We had to copy your commits into a new branch in the repository to run all our tests: see #6753. Once we get that new PR reviewed and approved, and all tests pass, we will merge that one in favor of this one. Your commits are still attributed to you as the author, though. Thanks for your help with this issue.


public static TBasicProperties CreateHeaders()
=> (TBasicProperties)Activator.CreateInstance();
public static TBasicProperties CreateHeaders(object readonlyBasicProperties)
=> (TBasicProperties)Activator(readonlyBasicProperties);
}
32 changes: 30 additions & 2 deletions ...ace/ClrProfiler/AutoInstrumentation/RabbitMQ/PopulateBasicPropertiesHeadersIntegration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// <copyright file="PopulateBasicPropertiesHeadersIntegration.cs" company="Datadog">
// <copyright file="PopulateBasicPropertiesHeadersIntegration.cs" company="Datadog">
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
// </copyright>
Expand Down Expand Up @@ -31,6 +31,12 @@ namespace Datadog.Trace.ClrProfiler.AutoInstrumentation.RabbitMQ;
[EditorBrowsable(EditorBrowsableState.Never)]
public class PopulateBasicPropertiesHeadersIntegration
{
internal static CallTargetState OnMethodBegin<TTarget, TBasicProperties, TActivity>(TTarget instance, TBasicProperties basicProperties, TActivity sendActivity, ulong publishSequenceNumber)
where TBasicProperties : IReadOnlyBasicProperties, IDuckType
{
return new CallTargetState(null, basicProperties);
}

internal static CallTargetReturn<TReturn?> OnMethodEnd<TTarget, TReturn>(TTarget instance, TReturn returnValue, Exception? exception, in CallTargetState state)
{
var tracer = Tracer.Instance;
Expand All @@ -45,7 +51,29 @@ public class PopulateBasicPropertiesHeadersIntegration
return new CallTargetReturn<TReturn?>(returnValue);
}

returnValue ??= CachedBasicPropertiesHelper<TReturn>.CreateHeaders();
// PopulateBasicPropertiesHeaders returns null if the supplied IReadOnlyBasicProperties
// does not have to be modified or if it's a writable instance.
// If that is the case then we have to fetch IReadOnlyBasicProperties from the argument
// list instead of creating a new instance that overwrites the supplied properties.
if (returnValue is null)
{
// Use the existing basic properties if possible,
// if not create new instance using the copy constructor on BasicProperties.
if (state.State.DuckIs<IBasicProperties>())
{
returnValue = (TReturn)state.State!;
}
else if (state.State.DuckIs<IReadOnlyBasicProperties>())
{
returnValue = CachedBasicPropertiesHelper<TReturn>.CreateHeaders(state.State!);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to preserve / copy headers here?
Haven't tested it out if that is necessary though

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look and see if I can sort it out. We definitely need to copy the basic properties and headers as they will be dropped completely from the message otherwise. For example the current implementation completely breaks our rabbitmq message sender if instrumentation is enabled as the messageid, timestame, persistance settings will be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
And let us know if need help on this as we definitely want to fix this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a commit that should fix the activator, not super happy about the cast but also not sure how to get rid of it. But I suppose optimizations can come after it's working properly :)

else
{
// This case cannot happen as argument is not nullable.
return new CallTargetReturn<TReturn?>(returnValue);
}
}

var duckType = returnValue.DuckCast<IBasicProperties>()!;

// add distributed tracing headers to the message
Expand Down
30 changes: 27 additions & 3 deletions tracer/test/test-applications/integrations/Samples.RabbitMQ/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public static class Program
private static readonly string exchangeName = "test-exchange-name";
private static readonly string routingKey = "test-routing-key";
private static readonly string queueName = "test-queue-name";
private static readonly string customHeaderName = "x-custom-header";

private static string Host()
{
Expand Down Expand Up @@ -65,6 +66,8 @@ private static async Task PublishAndGet(bool useDefaultQueue)
string publishExchangeName;
string publishQueueName;
string publishRoutingKey;
string messageId = Guid.NewGuid().ToString();
string headerValue = Guid.NewGuid().ToString();

using (SampleHelpers.CreateScope(messagePrefix))
{
Expand Down Expand Up @@ -95,12 +98,21 @@ await channel.QueueDeclareAsync(queue: publishQueueName,
// Test an empty BasicGetResult
await channel.BasicGetAsync(publishQueueName, true);

// Setup basic properties to verify instrumentation preserves properties and headers.
var properties = new BasicProperties
{
MessageId = messageId,
Headers = new Dictionary<string, object>()
{
{ customHeaderName, headerValue }
}
};

// Send message to the default exchange and use new queue as the routingKey
string message = $"{messagePrefix} - Message";
var body = Encoding.UTF8.GetBytes(message);
await channel.BasicPublishAsync(exchange: publishExchangeName,
routingKey: publishRoutingKey,
body: body);

await channel.BasicPublishAsync(publishExchangeName, publishRoutingKey, false, properties, body);
Console.WriteLine($"BasicPublish - Sent message: {message}");
}

Expand All @@ -110,6 +122,18 @@ await channel.BasicPublishAsync(exchange: publishExchangeName,
var result = await channel.BasicGetAsync(publishQueueName, true);
var resultMessage = Encoding.UTF8.GetString(result.Body.ToArray());
Console.WriteLine($"[Program.PublishAndGetDefault] BasicGet - Received message: {resultMessage}");

if (result.BasicProperties.MessageId != messageId)
{
throw new Exception("MessageId was not preserved in BasicProperties");
}

if (!result.BasicProperties.Headers.TryGetValue(customHeaderName, out var receivedHeaderValue)
|| receivedHeaderValue is not string receivedHeaderValueString
|| receivedHeaderValueString != headerValue)
{
throw new Exception("Custom header was not preserved in BasicProperties");
}
}
}

Expand Down
Loading