UriHelper.GetDisplayUrl() throws NullReferenceException if request is lacking host header

[aspnet-hello]

From @mikeharder on Tuesday, July 12, 2016 4:42:47 PM

If a client makes a valid HTTP 1.0 request without a host header, UriHelper.GetDisplayUrl() throws NullReferenceException.

Server Code

public class Program
{
    public static void Main(string[] args)
    {
        new WebHostBuilder()
            .UseKestrel(options =>
            {
                options.UseConnectionLogging();
            })
            .UseStartup<Program>()
            .Build()
            .Run();
    }

    public void Configure(IApplicationBuilder app, ILoggerFactory loggerFactory)
    {
        loggerFactory.AddConsole(LogLevel.Trace);
        var logger = loggerFactory.CreateLogger("Default");

        app.Run(async context =>
        {
            await context.Response.WriteAsync(context.Request.GetDisplayUrl());
        });
    }
}

Client Commands

> curl --http1.0 http://localhost:5000
http://localhost:5000/

> curl -v -H "Host:" --http1.0 http://localhost:5000
HTTP/1.1 500 Internal Server Error

Server Log

System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.AspNetCore.Http.Extensions.UriHelper.GetDisplayUrl(HttpRequest request)

Copied from original issue: aspnet/HttpAbstractions#671

[aspnet-hello]

From @Tratcher on Tuesday, July 12, 2016 7:55:55 PM

Was the Host header requirement a 1.1 addition?

[aspnet-hello]

From @mikeharder on Wednesday, July 13, 2016 12:00:45 PM

Yes, it's optional in 1.0: https://www.w3.org/Protocols/rfc2616/rfc2616-sec19.html#sec19.6.1

[aspnet-hello]

From @Tratcher on Wednesday, July 13, 2016 12:08:07 PM

Hmm, this method can't generate a complete url without the Host header. Should it throw InvalidOperationException or return an incomplete url like /path?query or http://<unknownhost>/path?query

[aspnet-hello]

From @mikeharder on Wednesday, July 13, 2016 12:12:38 PM

Exception seems bad, since every call to this method would need to be wrapped in a try/catch, since the app developer can't control whether clients will send a host header.

Could the method use something like Dns.GetHostName() or an IP address if there's no host header?

[aspnet-hello]

From @Tratcher on Wednesday, July 13, 2016 12:30:22 PM

LocalIp and port are OK, but also optional for servers. They'll also be different when you're behind IIS/Express.

Guessing is likely to confuse the user. I'd rather indicate that we didn't know.

[aspnet-hello]

From @mikeharder on Wednesday, July 13, 2016 12:37:38 PM

I would just use the incomplete URL then (/path?query). At least this is a valid (relative) URL.

[aspnet-hello]

From @Tratcher on Wednesday, July 13, 2016 1:26:33 PM

Yeah. The biggest downside is that it drops information you do know, the scheme.

[aspnet-hello]

From @mikeharder on Wednesday, July 13, 2016 2:05:15 PM

Agreed, but since there's no standard for "scheme and path without host", I think /path?query is better than something we make up like scheme://<unknownhost>/path?query.

[aspnet-hello]

From @muratg on Monday, July 25, 2016 2:14:01 PM

Backlogging for now. If we get enough customer complaint/request, we may reconsider.

[aspnet-hello]

From @Flavien on Sunday, June 25, 2017 5:30:41 PM

This issue also manifests if you use IHttpContextAccessor, which under specific circumstances will return an HttpContext that doesn't have a Host property defined.

[aspnet-hello]

From @rynowak on Sunday, June 25, 2017 5:49:41 PM

Are you trying to do this on a thread that isn't handling a request? None of this stuff is thread safe.

[aspnet-hello]

From @Flavien on Sunday, June 25, 2017 5:53:03 PM

Yes, I'm doing this on a thread that isn't handling a request, so I'm not sure why IHttpContextAccessor isn't returning null in the first place.

What's not thread safe and why should that have to be thread safe?

[aspnet-hello]

From @Flavien on Sunday, June 25, 2017 5:59:35 PM

Ok I see what's going on: I'm creating a Task from a request, which continues after the request has completed, so IHttpContextAccessor thinks I'm running this as part of the request, and because the request has already completed, the HttpContext that it returns is messed up.

[aspnet-hello]

From @Flavien on Monday, June 26, 2017 3:57:39 AM

I am puzzled, how can I start a background task from a request without capturing the HttpContext (so that IHttpContextAccessor returns null).

I tried everything, and every time, IHttpContextAccessor returns the old HttpContext.

[aspnet-hello]

From @rynowak on Monday, June 26, 2017 5:04:44 AM

I am puzzled, how can I start a background task from a request without capturing the HttpContext (so that IHttpContextAccessor returns null).

I tried everything, and every time, IHttpContextAccessor returns the old HttpContext.

If you want to do some work on a background task you need to capture all of the data you want to see up front and then spawn your task. Doing any access to an HttpContext on a randomly spawned task is going to continue to cause issues for you.

If you can share a little more about what you're trying to accomplish we can provide some suggestions.

Keep in mind, the server is already massively parallel (many requests) so attempting to parallelize the work of a single request is usually an overall loss of performance.

[aspnet-hello]

From @Flavien on Monday, June 26, 2017 6:22:38 AM

I don't care about performance, I just need the request to complete and return 200 to the client while a long running operation continues running on the server for a couple of minutes. This is fairly standard asynchronous API design.

I'm trying to access the HttpContext from a custom ILogger implementation that adds the current request path to the log output if this is being logged as part of a request. Of course, I need to be able to detect wether the logging is not being done as part of the request (so that I don't access an invalid HttpRequest). Unfortunately, IHttpContextAccessor always returns an HttpContext (sometimes the context of a completely unrelated request), even if not called as part of a request. To be perfectly clear, I don't want to access the HttpContext, I just want to be able to know whether I am in a request or not.

I haven't found any way to spawn a standalone task detached from the original request.

[aspnet-hello]

From @Tratcher on Monday, June 26, 2017 7:06:01 AM

ThreadPool.UnsafeQueueUserWorkItem?

[aspnet-hello]

From @Flavien on Monday, June 26, 2017 7:09:42 AM

This doesn't seem to exist on .NET Core.

[aspnet-hello]

From @davidfowl on Monday, June 26, 2017 10:23:44 AM

@Flavien unfortunately, until .NET Core 2.0 there isn't an easy way to avoid capturing the async local. Here's a sample of what you could do:

public class ThreadPoolImpl : IThreadPool
{
    private readonly IHttpContextAccessor _httpContextAccessor;

    public ThreadPoolImpl(IHttpContextAccessor httpContextAccessor)
    {
        _httpContextAccessor = httpContextAccessor;
    }

    public void QueueUserWorkItem(Func<Task> workitem)
    {
        _ = Execute(workitem);
    }

    private async Task Execute(Func<Task> workItem)
    {
        _httpContextAccessor.HttpContext = null;
        await workItem();
    }
}

public interface IThreadPool
{
    void QueueUserWorkItem(Func<Task> workItem);
}

Basically just set the HttpContext to null before executing the work item. That logic can also be isolated into a service so that you can swap the impl out for a simpler one in .NET Core .2.0

[aspnet-hello]

From @Flavien on Monday, June 26, 2017 2:25:18 PM

@davidfowl Thanks that seems to work. What are the mechanics behind HttpContextAccessor and AsyncLocal? It seems to revert to its initial value after Execute has returned. Is that behavior documented somewhere?

[aspnet-hello]

From @Volak on Friday, December 15, 2017 5:55:12 PM

@muratg I want to make a note - hopefully changing the "backlog" status.

HaProxy by default will use HTTP/1.0 to do health checks on http servers. See: https://www.haproxy.com/documentation/aloha/7-0/traffic-management/lb-layer7/health-checks/#check-http-service

This bug causes the service to report NullReferenceExceptions each and every health check

�[40m�[32minfo�[39m�[22m�[49m: Microsoft.AspNetCore.Hosting.Internal.WebHost[1]
      Request starting HTTP/1.0 GET http:///health  

System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.AspNetCore.Http.Extensions.UriHelper.GetDisplayUrl(HttpRequest request)
   at ServiceStack.AppHostBase.<ProcessRequest>d__11.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Hosting.Internal.RequestServicesContainerMiddleware.<Invoke>d__3.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Frame`1.<ProcessRequestsAsync>d__2.MoveNext()

There is a workaround - instead of using the config line option httpchk GET /health you would do option httpchk GET /health HTTP/1.1

but in my case I have little to no control of haproxy config as its set by another package. I'll have to investigate if there's a way to change this line in their configs

[aspnet-hello]

From @Tratcher on Friday, December 15, 2017 5:58:43 PM

https://github.com/aspnet/Hosting/issues/1289 would address this.

[yogiraj07]

Hello,
AWS X-Ray SDK for .NET/Core is facing the same issue and this is breaking customer application when host is not present in the request header.
This behavior is for ASP.NET Core 2.x.
I do see a commit in .NET Core 3.0 which fixes this issue. Are you planning to have same behavior for GetDisplayUrl(HttpRequest request) for NET Core 2.0? If yes, any ETA?

We need to fix this at earliest and for the moment I can retrieve the URL using this code, by manually copy pasting. However, I want to avoid this, considering this is downstream dependency issue and want to prevent behavioral changes for the customers between ASP.NET Core 2.x and 3.x.

Any updates are highly appreciated.

Thanks,
Yogi

[Tratcher]

I didn't realize this issue was still open. Closing as resolved. PR: aspnet/HttpAbstractions#1057

@yogiraj07 this was not being considered for 2.x servicing. @anurse?

[analogrelay]

@yogiraj07 Do you have any information as to the customer impact? (i.e. How many customers you've seen with this issue?). With 3.0 scheduled for the end of the year the bar for backporting to 2.x is pretty high but this is a simple change so it may be feasible if we know more about the customer impact.

[yogiraj07]

@anurse ,
Thanks for quick response.
Currently 1 issue is being reported. However, since the X-Ray SDK is in the hot code path of the customer code application, this may cause larger impact in near future which is highly undesirable.

I have 2 options in my mind -

1. Copy paste the Code and get URL for any Net Core version and not at all rely on GetDisplayUrl provided by the NET Core API.
2. Add try catch around GetDisplayUrl(HttpRequest request) inside X-Ray SDK call site and return empty URL.

The thing is

1. I can go with solution 1, however, the core logic of getting URL is copy paste and it adds maintenance overhead. If further going ahead, there is any behavioral change in getting URL from your side, the X-Ray team had to back fill it and usually customer may need to notify us. I am trying to avoid this and asking here if we can get this ported on NET Core 2.0 platform.

2. The issue with solution 2 : This will be behavioral change in the way URL is retrieved. For example, if host is not present, NET Core 2.0 will return url = "" while NET Core 3.0 will return URL= "http:///" (this is generated by this commit). This will impact customer analysis for 2 different platforms.

Any thoughts on this?

[yogiraj07]

Hi @anurse , @Tratcher ,
Can you please provide any update to my previous post. I would like to take necessary steps on our side to mitigate this issue.

Thanks,
Yogi

[analogrelay]

We haven't been able to take this for approval for backporting yet as our window for doing so hasn't opened yet (it should open in the next week or so). If it's approved it'll still be June (at best) before the patch is released.

As to how you can work around it, I think that copying the code is your best option here. As you can see, the logic is not very complicated and looking at the history it has had very minimal behavior changes over time.