HttpSys request header dictionary does not remove headers when Remove() is called

[markalward]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

When using the HttpSys server, the request header dictionary (HttpRequest.Headers) behaves unexpectedly when trying to remove a header using IHeaderDictionary.Remove(). The Remove() method fails to remove some headers, even when those headers are present in the request - when enumerating the collection after the call to Remove(), the original header is still returned.

Expected Behavior

If a header is present in the header dictionary, it should be removed from the collection when Remove() is called. Subsequent operations on the collection (enumeration, indexer, ContainsKey, etc.) should not return the removed header.

Steps To Reproduce

Create an ASP.NET Core app, using HttpSys as the server. The code below reproduces the issue.

in Startup.cs:

public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
{
    app.Run(httpContext =>
    {
        var headers = httpContext.Request.Headers;
        bool removed = headers.Remove("Connection");

        Debug.WriteLine(removed); // prints 'false'

        string connection = headers["Connection"];
        Debug.WriteLine(connection); // prints 'Close'

        return Task.CompletedTask;
    });
}

in Program.cs:

var builder = Host.CreateDefaultBuilder(args)
    .ConfigureWebHostDefaults(webBuilder =>
    {
        webBuilder.UseHttpSys(options =>
        {
            options.UrlPrefixes.Add("http://+:5000/");
        });

        webBuilder.UseStartup<Startup>();
    });

var app = builder.Build();
app.Run();

This curl command was used:
curl --verbose --header Connection:Close http://localhost:5000/

Note that a number of other headers such as 'Accept-Encoding' also reproduce the issue.

Exceptions (if any)

No response

.NET Version

net6.0

Anything else?

Relevant line of code in RequestHeaders implementation: https://github.com/dotnet/aspnetcore/blob/main/src/Shared/HttpSys/RequestProcessing/RequestHeaders.Generated.cs#L1860

If the _Connection field hasn't been populated at this point in the code, PropertiesTryRemove() returns false and the header isn't removed. Later on, if the same header is retrieved using PropertiesTryGetValue(), the actual header value is retrieved by calling NativeRequestContext.GetKnownHeader().

Other details:

• This issue appears to affect all the 'known' headers listed in the RequestHeaders.Generated.cs file.
• The issue does not repro if the header was previously loaded, due to enumerating the header dictionary or retrieving the header using request.Headers["Connection"].
• The issue does not repro on Kestrel.

[Tratcher]

Note this is generated code. The template is here: RequestHeaders.Generated.tt

I don't think anything is set up to actually run that template anymore. See #5869.

[singh733]

@Tratcher Please see the proposal below:

Code from HttpSysServer

case 10:
                    if (_Connection.Count > 0
                        && string.Equals(key, HeaderNames.Connection, StringComparison.Ordinal))
                    {
		         bool wasSet = ((_flag0 & 0x80u) != 0);
                        Connection = StringValues.Empty;
                        return wasSet;

Code from Kestrel

case 10:
                {
                    if (ReferenceEquals(HeaderNames.Connection, key))
                    {
                        if ((_bits & 0x2L) != 0)
                        {
                            _bits &= ~0x2L;
                            _headers._Connection = default(StringValues);
                            return true;
                        }
                        return false;
                    }

Solution:

Make logic same as kestrel

1. We can remove Connection.Count in HttpSysServer
2. If Property is not read then make it StringValues.Empty and return true , otherwise return false

Note: This change should be in the .tt files .

[Tratcher]

_flag0 might also need updating. Feel free to try it.

[singh733]

@Tratcher Just wondering that there is NO .tt file in kestrel server for the HttpHeaders.Generated.cs.

I need the .tt file in kestrel because i want to compare it with .tt file in Httpsys.
Please let me know if i miss anything.

I have asked question regarding that in Discord.

[Tratcher]

The .tt file in Http.Sys is part of a much older code generation tool that isn't set up correctly to work right now. We'll have to fix or replace it. See #5869.

Kestrel uses a different tool that we could adapt for use in Http.Sys:
https://github.com/dotnet/aspnetcore/tree/main/src/Servers/Kestrel/tools/CodeGenerator

[singh733]

@Tratcher I have changed the code in .tt file in http.Sys. I have made the functions same as Kestrel. Below are the code changes that I have done in RequestHeaders.Generated.tt

a) Change the functions

Func<int,string> IsRead = Index => "((_flag & 0x" + (1L << Index).ToString("x", CultureInfo.InvariantCulture) + "L) != 0)";
Func<int,string> MarkRead = Index => "_flag |= 0x" + (1L << Index).ToString("x", CultureInfo.InvariantCulture) + "L";
Func<int,string> Clear = Index => "_flag &= ~0x" + (1L << Index).ToString("x", CultureInfo.InvariantCulture) + "L";

b) Change the datatype from uint to long

private long _flag;

c) Remove the count condition

if (string.Equals(key, HeaderNames.<#=prop.Name#>, StringComparison.Ordinal))

Please see the screenshot below of changes

One thing I am not able to understand why these type of conditions ( for example : (_flag & 0x1000000000L) != 0)) are false in http.sys and same type of conditions ( for example : (_bits & 0x4000000000L) != 0) are true in Kestrel.

Would you please help me on this.

[Tratcher]

This might be because Http.Sys hasn't loaded that header yet. Kestrel loads all headers eagerly, but Http.Sys leaves the headers in the native structure until someone asks for them. If you read that header first then the bits should be set. That difference is the source of this removal bug, we're trying to remove a value that hasn't been loaded yet, and we mistakenly assume it doesn't exist.

[singh733]

@Tratcher Yes, you are correct, Few headers are eagerly loaded in kestrel but not all headers. For example : AcceptLanguage header didn't eagerly load still it returns true in kestrel. Please correct me if I am wrong

Note:

Few headers are eagerly loaded in both http.sys and kestrel. For example : Host header

[singh733]

@Tratcher I dig more into the code and below are my findings:

a) Headers which eagerly loads returns true and if not then returns false. It is same for both Kestrel and httpsys. I don't think there is any problem with .tt file in httpsys. For example:

1. This code returns removed = true in httpsys.

public void Configure(IApplicationBuilder app)
    {
        app.Run(async context =>
        {
            var headers = context.Request.Headers;
            
            bool removed = headers.Remove("Host");
            //Removed is true

2. This code return removed = false in kestrel

app.Run(async context =>
        {
            var headers = context.Request.Headers;
           
            bool removed = headers.Remove("Accept-Charset");
            //Removed false

Now I am looking into why more headers are eagerly load in kestrel than httpsys.