Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix WhiteSpace validation in HttpHeaders #102693

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

MihaZupan
Copy link
Member

Fixes #102584

#86007 added a bit too much validation for header values. In a couple of places it was redundant with the checks we would do anyway (e.g. right before calling HeaderDescriptor.TryGet or HeaderUtilities.CheckValidToken), and in a few others it was more strict than the parsing validation that only looks for the ASCII space and horizontal tab.

In a couple of places we were also calling into validating constructors right after parsing, thus wasting cycles on validating twice.

@MihaZupan MihaZupan added this to the 9.0.0 milestone May 25, 2024
@MihaZupan MihaZupan requested a review from a team May 25, 2024 22:26
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@MihaZupan
Copy link
Member Author

@MihuBot fuzz HttpHeaders

@MihaZupan

This comment was marked as resolved.

@EgorBot

This comment was marked as resolved.

@MihaZupan
Copy link
Member Author

@EgorBot

using System;
using System.Net.Http;
using System.Net.Http.Headers;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkRunner.Run<HeaderBenchmarks>(args: args);
//BenchmarkRunner.Run<HeaderBenchmarks>();

public class HeaderBenchmarks
{
    private readonly HttpRequestHeaders _request = new HttpRequestMessage().Headers;
    private readonly HttpResponseHeaders _response = new HttpResponseMessage().Headers;

    [Benchmark]
    public void Warning()
    {
        _request.Add("Warning", "001 Hello \"World\"");
        _request.Clear();
    }

    [Benchmark]
    public void ETag()
    {
        _response.Add("ETag", "\"foo\"");
        _response.Clear();
    }
}

@EgorBot
Copy link

EgorBot commented May 29, 2024

Benchmark results on Intel
BenchmarkDotNet v0.13.12, Ubuntu 22.04.4 LTS (Jammy Jellyfish)
Intel Xeon Platinum 8370C CPU 2.80GHz, 1 CPU, 16 logical and 8 physical cores
  Job-ZQKWLS : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
  Job-VPTTUM : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
Method Toolchain Mean Error Ratio
Warning Main 130.19 ns 0.402 ns 1.00
Warning PR 103.31 ns 0.428 ns 0.79
ETag Main 67.21 ns 0.273 ns 1.00
ETag PR 55.91 ns 0.469 ns 0.83

BDN_Artifacts.zip

@EgorBot
Copy link

EgorBot commented Jun 4, 2024

EgorBot manual
Usage: @EgorBot [-%target%] [-profiler] [raw args for BDN] `C# snippet surrounded with triple ticks`
-%target%:       Can be -arm64, -amd or -intel. Or multiple at once, e.g. '-amd -intel'
                 -intel is used when none of the targets are specified.
-profiler:       Use 'perf record' to collect a flamegraph/hot asm - shouldn't be used 
                 when the given benchmark snippet contains more than one [Benchmark]
                 Disabled by default.
-mono:           Use Mono runtime instead of CoreCLR for all targets. Should be possible to use
                 Mono interp too (LLVM is not supported yet).
                 Mono doesn't support -profiler (at least JIT)
                 To use mono-interp, use BDN args, e.g. --envvars MONO_ENV_OPTIONS:"--interpreter"
                 Disabled by default.
-[args for BDN]: Args directly passed to BDN e.g. '--disasm', see
                 https://github.com/dotnet/BenchmarkDotNet/blob/master/docs/articles/guides/console-args.md

All targets are Linux-only at the moment.
NOTE: BenchmarkRunner.Run or BenchmarkSwitcher.From* can be omitted (snippet without an entrypoint)
Although, if they're presented then Program's args must be be forwarded to Run(args: args)

NOTE: [DisassemblyDiagnoser] may cause unexpected crashes in BDN on Linux (at least on x64)

Usage example: link

@MihaZupan
Copy link
Member Author

@dotnet/ncl any volunteers for reviewing this one?

@liveans
Copy link
Member

liveans commented Jun 4, 2024

@dotnet/ncl any volunteers for reviewing this one?

I will, but would be better to have one more eye on this

Copy link
Member

@liveans liveans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a question

@MihaZupan MihaZupan merged commit e2bc045 into dotnet:main Jun 10, 2024
83 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpHeaders TryParse can throw
3 participants