fix stack overflow when overriding HttpClientHandler.SendAsync() #367

lucaspimentel · 2019-05-31T20:05:34Z

Fix StackOverflowException when HttpClientHandler.SendAsync() is overridden by a method that calls the base method. See #351.

For example:

public class DerivedHandler : HttpClientHandler
{
    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        return await base.SendAsync(request, cancellationToken);
    }
}

This PR makes changes that affect all integrations:

profiler: push new argument (the original call's opcode) to execution stack when replacing a method call
integrations: add new opcode argument to every wrapper method to pop this value off the stack
most integration won't use this yet and default to CALLVIRT to keep the current behavior

Changes to HttpMessageHandlerIntegration:

split into separate wrappers for calls to HttpMessageHandler.SendAsync() and HttpClientHandler.SendAsync()
use the new opcode argument to emit the correct IL (CALL or CALLVIRT) when calling original method

…emitting IL

…call's opcode (value not used yet)

…pcode argument to emit the correct IL

lucaspimentel · 2019-05-31T20:39:59Z

src/Datadog.Trace.ClrProfiler.Managed/Emit/DynamicMethodBuilder.cs

@@ -329,18 +336,22 @@ internal static class DynamicMethodBuilder<TDelegate>
                }
            }

-            if (methodInfo.IsStatic)
+            if (callOpCode == OpCodeValue.Call || methodInfo.IsStatic)


This is where we use the new opcode value when emitting IL. Most integrations will still pass the default opcode (CALLVIRT), so we still check if it's a static method and use CALL.

lucaspimentel · 2019-05-31T20:40:07Z

src/Datadog.Trace.ClrProfiler.Native/cor_profiler.cpp

+      // additional arguments for the wrapper method
+      ILRewriterWrapper rewriter_wrapper(&rewriter);
+      rewriter_wrapper.SetILPosition(pInstr);
+      rewriter_wrapper.LoadInt32(pInstr->m_opcode);


This is where we insert the original call's opcode value into the execution stack before replacing the method call.

zacharycmontoya · 2019-05-31T21:19:40Z

src/Datadog.Trace.ClrProfiler.Managed/Integrations/HttpMessageHandlerIntegration.cs

            var tokenSource = cancellationTokenSource as CancellationTokenSource;
            var cancellationToken = tokenSource?.Token ?? CancellationToken.None;
+            var callOpCode = (OpCodeValue)opCode;
+            var handlerType = typeof(HttpClientHandler); // Note the HttpClientHandler to match the method call we replaced


Could you rename handlerType to httpClientHandlerType to be a little more specific about the meaning of the variable? I was confused initially because I thought handlerType meant handler.GetType()

Renamed that and a few more in feffd93.

That helps a lot, thanks!

zacharycmontoya · 2019-05-31T21:29:44Z

src/Datadog.Trace.ClrProfiler.Managed/Integrations/HttpMessageHandlerIntegration.cs

+            var tokenSource = cancellationTokenSource as CancellationTokenSource;
+            var cancellationToken = tokenSource?.Token ?? CancellationToken.None;
+            var callOpCode = (OpCodeValue)opCode;
+            var handlerType = typeof(HttpMessageHandler); // Note the HttpMessageHandler to match the method call we replaced


Same comment here, can we do httpMessageHandlerType instead of just handlerType for the variable name?

Renamed in the same commit as the previous comment.

colin-higgins · 2019-06-03T13:49:07Z

Moving forward with this then, our convention will be that all wrapper methods must have int opCode as the last argument?
Sounds like a good candidate for a test as well, something along the lines of:
wrapper_methods.Select(w => w.MethodInfo).All(m => m.Parameters.Last().GetType() == typeof(int));

colin-higgins · 2019-06-03T14:09:14Z

samples/Directory.Build.props

@@ -17,7 +17,7 @@
  </ItemGroup>

  <ItemGroup>
-    <None Include="..\..\src\Datadog.Trace.ClrProfiler.Native\bin\$(Configuration)\$(Platform)\**"
+    <None Include="..\..\src\Datadog.Trace.ClrProfiler.Native\bin\$(Configuration)\$(Platform)\*.dll;..\..\src\Datadog.Trace.ClrProfiler.Native\bin\$(Configuration)\$(Platform)\*.so"


Doesn't this disregard pdb files?

Good catch! I added the pdb files in 2f28ef3.

…ameter

lucaspimentel · 2019-06-03T15:31:45Z

@colin-higgins: (sorry, I couldn't reply directly to your comment)

our convention will be that all wrapper methods must have int opCode as the last argument?

Yes. As of this PR only the HttpClientHandler integration uses this new value, and the other integrations ignore it and keep using the old behavior. I will create a separate PR to change all the other integrations to use this value. Using the original opcode (CALL vs CALLVIRT) is the more correct behavior and we should do it across the board to avoid hitting this bug again if we instrument another overridden virtual method.

Sounds like a good candidate for a test as well

Good point! I added a test in ba059d4.

lucaspimentel added 5 commits May 31, 2019 11:29

add app that reproduces issue

1b2c791

update props to only include dll/so files

067f745

clean up comments

e0ca8cc

add opcode parameter to DynamicMethodBuilder methods and use it when …

ac9ea13

…emitting IL

add an additional argument to every wrapper method with the original …

c3c42f9

…call's opcode (value not used yet)

lucaspimentel added type:bug area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) labels May 31, 2019

lucaspimentel added this to the 1.3 milestone May 31, 2019

lucaspimentel self-assigned this May 31, 2019

split HttpMessageHandlerIntegration into separate wrappers, use new o…

1e5e290

…pcode argument to emit the correct IL

lucaspimentel force-pushed the lpimentel/httpclient-stackoverflow branch from 084b4f8 to 1e5e290 Compare May 31, 2019 20:09

lucaspimentel added 2 commits May 31, 2019 16:10

keep using CALL for static method calls

42dfbd3

fix build error in AutomapperTest project

38d1bd0

lucaspimentel commented May 31, 2019

View reviewed changes

lucaspimentel marked this pull request as ready for review May 31, 2019 20:40

lucaspimentel requested a review from a team as a code owner May 31, 2019 20:40

zacharycmontoya reviewed May 31, 2019

View reviewed changes

lucaspimentel added 2 commits May 31, 2019 17:21

fix integrations.json

0dea279

rename several variables for clarity

feffd93

zacharycmontoya reviewed May 31, 2019

View reviewed changes

zacharycmontoya approved these changes May 31, 2019

View reviewed changes

colin-higgins reviewed Jun 3, 2019

View reviewed changes

lucaspimentel added 2 commits June 3, 2019 10:31

include profiler pdb files in sample projects

2f28ef3

add test to ensure all wrapper methods have the last "int opCode" par…

ba059d4

…ameter

lucaspimentel merged commit cf8326c into develop Jun 3, 2019

lucaspimentel deleted the lpimentel/httpclient-stackoverflow branch June 3, 2019 16:35

lucaspimentel mentioned this pull request Jun 3, 2019

pass opcode when using Emit.DynamicMethodBuilder<T> #368

Closed

lucaspimentel mentioned this pull request Jul 16, 2019

use original opcode in DynamicMethodBuilder #448

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix stack overflow when overriding HttpClientHandler.SendAsync() #367

fix stack overflow when overriding HttpClientHandler.SendAsync() #367

lucaspimentel commented May 31, 2019 •

edited

Loading

lucaspimentel May 31, 2019

lucaspimentel May 31, 2019

zacharycmontoya May 31, 2019

lucaspimentel May 31, 2019

zacharycmontoya May 31, 2019

zacharycmontoya May 31, 2019

lucaspimentel May 31, 2019

colin-higgins commented Jun 3, 2019

colin-higgins Jun 3, 2019

lucaspimentel Jun 3, 2019

lucaspimentel commented Jun 3, 2019

fix stack overflow when overriding HttpClientHandler.SendAsync() #367

fix stack overflow when overriding HttpClientHandler.SendAsync() #367

Conversation

lucaspimentel commented May 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-higgins commented Jun 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaspimentel commented Jun 3, 2019

lucaspimentel commented May 31, 2019 •

edited

Loading