Fix: Rabbitmq7 header injection overwrites user supplied basic properties #6730
Conversation
|
@johang88: Thanks for this! We'll take a look, run all tests, and merge it if all goes well. |
| 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); |
There was a problem hiding this comment.
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()
| } | ||
| else if (state.State.DuckIs<IReadOnlyBasicProperties>()) | ||
| { | ||
| returnValue = CachedBasicPropertiesHelper<TReturn>.CreateHeaders(state.State!); |
There was a problem hiding this comment.
I wonder if we need to preserve / copy headers here?
Haven't tested it out if that is necessary though
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you!
And let us know if need help on this as we definitely want to fix this
There was a problem hiding this comment.
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 :)
| il.Emit(OpCodes.Castclass, parameterType); // Cast to IReadOnlyBasicProperties | ||
| il.Emit(OpCodes.Newobj, constructor); | ||
| il.Emit(OpCodes.Ret); | ||
| Activator = (Func<object, object>)createBasicPropertiesMethod.CreateDelegate(typeof(Func<object, object>), targetType); |
There was a problem hiding this comment.
@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.
…c properties [from external PR #6730] (#6753) Based on #6730 by @johang88. ## Summary of changes Fixes the header injection so that basic properties are preserved from the supplied argument list in case `PopulateBasicPropertiesHeaders` returns null which happens if the basic properties argument is writable or if no changes had to be made to it. ## Reason for change Fixes a bug that would remove all user supplied basic properties. ## Implementation details Stores the basicproperties argument in the active scope and retrieves it a null return value is encountered. Changed `CachedBasicPropertiesHelper` to call the copy constructor that takes a `IReadOnlyBasicProperties` as argument. ## Test coverage Added basic properties to one of the rabbitmq7 test methods as well as assertions. ## Other details Fixes #6723 --------- Co-authored-by: Andrew Lock <[email protected]> Co-authored-by: Johan Gustafsson <[email protected]>
…c properties [from external PR #6730] (#6753) Based on #6730 by @johang88. ## Summary of changes Fixes the header injection so that basic properties are preserved from the supplied argument list in case `PopulateBasicPropertiesHeaders` returns null which happens if the basic properties argument is writable or if no changes had to be made to it. ## Reason for change Fixes a bug that would remove all user supplied basic properties. ## Implementation details Stores the basicproperties argument in the active scope and retrieves it a null return value is encountered. Changed `CachedBasicPropertiesHelper` to call the copy constructor that takes a `IReadOnlyBasicProperties` as argument. ## Test coverage Added basic properties to one of the rabbitmq7 test methods as well as assertions. ## Other details Fixes #6723 --------- Co-authored-by: Andrew Lock <[email protected]> Co-authored-by: Johan Gustafsson <[email protected]>
Note: Created as draft as I have not actually run or tested the changes 😶
Summary of changes
Fixes the header injection so that basic properties are preserved from the supplied argument list in case
PopulateBasicPropertiesHeadersreturns null which happens if the basic properties argument is writable or if no changes had to be made to it.Reason for change
Fixes a bug that would remove all user supplied basic properties.
Implementation details
Stores the basicproperties argument in the active scope and retrieves it a null return value is encountered. Changed
CachedBasicPropertiesHelperto call the copy constructor that takes aIReadOnlyBasicPropertiesas argument.Test coverage
Added basic properties to one of the rabbitmq7 test methods as well as assertions.
Other details
Fixes #6723