Protect against UriFormatException

[NachoEchevarria]

Summary of changes

We are getting the following error in error tracking:

Error : Datadog ASP.NET HttpModule instrumentation error
System.UriFormatException
at System.Uri.CreateThis(String uri, Boolean dontEscape, UriKind uriKind)
at System.Web.Util.UriUtil.BuildUriImpl(String scheme, String serverName, String port, String path, String queryString, Boolean useLegacyRequestUrlGeneration)
at System.Web.HttpRequest.BuildUrl(Func`1 pathAccessor)
at System.Web.HttpRequest.get_Url()
at Datadog.Trace.AspNet.TracingHttpModule.OnBeginRequest(Object sender, EventArgs eventArgs)

The error was not easy to reproduce since the request has to be initially correct so it actually reaches the server.

After some tests, the error could be reproduced by adding a new module in our sample page. This module modifies the request by adding a new server name, which ends up modifying the host name of the url.

public class ForwardedHostBreakModule : IHttpModule
{
    public void Init(HttpApplication app)
    {
        app.BeginRequest += (s, e) =>
        {
            var ctx = ((HttpApplication)s).Context;

            // Customer-likely path: If a proxy sent X-Forwarded-Host, copy it to SERVER_NAME
            var fwd = ctx.Request.Headers["X-Forwarded-Host"];
            if (!string.IsNullOrEmpty(fwd))
            {
                // This mutates what System.Web uses to assemble Request.Url
                ctx.Request.ServerVariables.Set("SERVER_NAME", fwd);
            }
        };
    }
    public void Dispose() { }
}

And in web.config:

  <system.webServer>
    <modules runAllManagedModulesForAllRequests="true">
      <add name="ForwardedHostBreakModule" type="ForwardedHostBreakModule" />
      </modules>
  </system.webServer>
</configuration>

If we send this request containing this header and value to a sample page containing the previous module, we will get the same exception that is shown in error tracking:

X-Forwarded-Host: localhost:4444ide

Reason for change

Implementation details

Test coverage

A new integration tests could be added by replicating the previous method. Not really fond of adding this overhead to our CI and sample apps in this case, since it's a corner case not so easy to reproduce.

Other details

[dd-trace-dotnet-ci-bot]

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

• This PR (7550)
• master

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

• Welch test with statistical test for significance of 5%
• Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7550) - mean (72ms)  : 71, 73
     .   : milestone, 72,
    master - mean (72ms)  : 71, 73
     .   : milestone, 72,

    section Baseline
    This PR (7550) - mean (68ms)  : 67, 70
     .   : milestone, 68,
    master - mean (68ms)  : 66, 70
     .   : milestone, 68,

    section CallTarget+Inlining+NGEN
    This PR (7550) - mean (1,049ms)  : 1005, 1093
     .   : milestone, 1049,
    master - mean (1,049ms)  : 988, 1110
     .   : milestone, 1049,

Loading

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7550) - mean (106ms)  : 105, 107
     .   : milestone, 106,
    master - mean (106ms)  : 105, 108
     .   : milestone, 106,

    section Baseline
    This PR (7550) - mean (106ms)  : 103, 108
     .   : milestone, 106,
    master - mean (105ms)  : 103, 108
     .   : milestone, 105,

    section CallTarget+Inlining+NGEN
    This PR (7550) - mean (745ms)  : 714, 775
     .   : milestone, 745,
    master - mean (745ms)  : 716, 774
     .   : milestone, 745,

Loading

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7550) - mean (94ms)  : 93, 95
     .   : milestone, 94,
    master - mean (94ms)  : 93, 95
     .   : milestone, 94,

    section Baseline
    This PR (7550) - mean (94ms)  : 91, 96
     .   : milestone, 94,
    master - mean (93ms)  : 91, 95
     .   : milestone, 93,

    section CallTarget+Inlining+NGEN
    This PR (7550) - mean (700ms)  : 676, 724
     .   : milestone, 700,
    master - mean (706ms)  : 677, 735
     .   : milestone, 706,

Loading

gantt
    title Execution time (ms) FakeDbCommand (.NET 8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7550) - mean (93ms)  : 91, 94
     .   : milestone, 93,
    master - mean (92ms)  : 91, 93
     .   : milestone, 92,

    section Baseline
    This PR (7550) - mean (92ms)  : 89, 95
     .   : milestone, 92,
    master - mean (92ms)  : 90, 94
     .   : milestone, 92,

    section CallTarget+Inlining+NGEN
    This PR (7550) - mean (659ms)  : 640, 677
     .   : milestone, 659,
    master - mean (664ms)  : 647, 680
     .   : milestone, 664,

Loading

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7550) - mean (202ms)  : 196, 207
     .   : milestone, 202,
    master - mean (199ms)  : 197, 202
     .   : milestone, 199,

    section Baseline
    This PR (7550) - mean (199ms)  : 193, 205
     .   : milestone, 199,
    master - mean (196ms)  : 193, 199
     .   : milestone, 196,

    section CallTarget+Inlining+NGEN
    This PR (7550) - mean (1,177ms)  : 1127, 1226
     .   : milestone, 1177,
    master - mean (1,170ms)  : 1101, 1240
     .   : milestone, 1170,

Loading

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7550) - mean (284ms)  : 279, 289
     .   : milestone, 284,
    master - mean (282ms)  : 275, 289
     .   : milestone, 282,

    section Baseline
    This PR (7550) - mean (284ms)  : 277, 290
     .   : milestone, 284,
    master - mean (281ms)  : 276, 286
     .   : milestone, 281,

    section CallTarget+Inlining+NGEN
    This PR (7550) - mean (962ms)  : 898, 1026
     .   : milestone, 962,
    master - mean (945ms)  : 900, 989
     .   : milestone, 945,

Loading

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7550) - mean (282ms)  : 270, 293
     .   : milestone, 282,
    master - mean (275ms)  : 269, 281
     .   : milestone, 275,

    section Baseline
    This PR (7550) - mean (282ms)  : 272, 292
     .   : milestone, 282,
    master - mean (275ms)  : 270, 279
     .   : milestone, 275,

    section CallTarget+Inlining+NGEN
    This PR (7550) - mean (940ms)  : 900, 980
     .   : milestone, 940,
    master - mean (930ms)  : 895, 965
     .   : milestone, 930,

Loading

gantt
    title Execution time (ms) HttpMessageHandler (.NET 8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7550) - mean (275ms)  : 269, 282
     .   : milestone, 275,
    master - mean (275ms)  : 265, 286
     .   : milestone, 275,

    section Baseline
    This PR (7550) - mean (276ms)  : 270, 282
     .   : milestone, 276,
    master - mean (273ms)  : 266, 281
     .   : milestone, 273,

    section CallTarget+Inlining+NGEN
    This PR (7550) - mean (866ms)  : 839, 892
     .   : milestone, 866,
    master - mean (861ms)  : 840, 883
     .   : milestone, 861,

Loading

[andrewlock]

Nice work! A couple of minor suggestions.

In terms of testing, if you wanted, you could potentially add the module to an existing sample, and control whether it does anything via an env variable. I'm generally OK with us not explicitly testing it as it's an edge case, my only tiny concern is accidental regressions

[NachoEchevarria]

Nice work! A couple of minor suggestions.

In terms of testing, if you wanted, you could potentially add the module to an existing sample, and control whether it does anything via an env variable. I'm generally OK with us not explicitly testing it as it's an edge case, my only tiny concern is accidental regressions

Actually, I found a way to unit test this, so we can test it with less overhead. I have added the test.

[NachoEchevarria]

Thanks for the feedback and review!