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 HTTP3 header decoder buffer allocation #78862

Merged
merged 9 commits into from
Apr 19, 2023
Merged

Conversation

BrunoBlanes
Copy link
Contributor

Fixes #78516

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 26, 2022
@ghost
Copy link

ghost commented Nov 26, 2022

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

Issue Details

Fixes #78516

Author: BrunoBlanes
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@dnfadmin
Copy link

dnfadmin commented Nov 26, 2022

CLA assistant check
All CLA requirements met.

@BrunoBlanes
Copy link
Contributor Author

BrunoBlanes commented Nov 26, 2022

Although this should fix the issue, I believe I've identified at least one other point of failure in the original code, so I am willing to run some more tests.

@BrunoBlanes
Copy link
Contributor Author

BrunoBlanes commented Nov 27, 2022

I added a few more tests and they all pass fine, so I did not find any other bug, however they run with a fixed sized header name and value, which in turn doesn't test how InternalDecoder behaves when the value length is over 127 bytes. Also, these tests only cover the Literal Field Without Name Reference case.

I'm aware of another test file of the same name located under the Common.Tests solution, however I wasn't able to get that running and also don't think these tests I've written are worth adding into the code base since they only cover a niche case, I've added them here mostly to shed a light into the investigation that's been done.

@MihaZupan
Copy link
Member

Thank you for the investigation work here and for even providing a fix.
Sorry about the review delay, I'll get to this change this week.

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

While looking more closely at this change, I realized that the same issue is present with the HPackDecoder (HTTP/2), it's just unlikely to be hit in practice as the initial size of the name buffer is 4 KB. As these files are quite similar, would you be willing to match the changes you made in QPackDecoder to HPackDecoder as well?

I'm aware of another test file of the same name located under the Common.Tests solution, however I wasn't able to get that running and also don't think these tests I've written are worth adding into the code base since they only cover a niche case, I've added them here mostly to shed a light into the investigation that's been done.

I think these tests are very valuable - they would have caught the bug if we had the before.

We shouldn't introduce a new QPackDecoderTest.cs file as the shared one already exists though. The HPackDecoder and QPackDecoder logic and their tests are shared between the runtime (HttpClient) and ASP.NET Core (Kestrel).

If you look at the System.Net.Http.Unit.Tests.csproj file, you can see how the product code and test code are linked into the project

<Compile Include="$(CommonPath)System\Net\Http\aspnetcore\Http2\Hpack\HPackDecoder.cs"
         Link="Common\System\Net\Http\aspnetcore\Http2\Hpack\HPackDecoder.cs" />
<Compile Include="$(CommonPath)..\tests\Tests\System\Net\aspnetcore\Http2\HPackDecoderTest.cs"
         Link="HPack\HPackDecoderTest.cs" />

We should be able to do the same for the QPackDecoder and its tests. Let me know if I can help with that.

@BrunoBlanes
Copy link
Contributor Author

While looking more closely at this change, I realized that the same issue is present with the HPackDecoder (HTTP/2), it's just unlikely to be hit in practice as the initial size of the name buffer is 4 KB. As these files are quite similar, would you be willing to match the changes you made in QPackDecoder to HPackDecoder as well?

Yes, I'd love to help with that.

We should be able to do the same for the QPackDecoder and its tests. Let me know if I can help with that.

I'll see if I can manage that, else I'll let you know to help me out.

@BrunoBlanes
Copy link
Contributor Author

I've been trying to do some debugging but am unable to attach:

System.Runtime.InteropServices.COMException (0x89710080): Unable to attach. A debug component is not installed.
   at Microsoft.VisualStudio.Shell.Interop.IVsDebuggerLaunchAsync.LaunchDebugTargetsAsync(UInt32 debugTargetCount, VsDebugTargetInfo4[] debugTargetArray, IVsDebuggerLaunchCompletionCallback completionCallback)
   at Microsoft.VisualStudio.TestWindow.ShellServices.DebuggerService.<>c__DisplayClass24_0.<<LaunchDebuggerAsync>b__0>d.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.VisualStudio.TestWindow.Logging.ILoggerExtensions.<CallWithCatchAsync>d__11`1.MoveNext()

Is there a known solution to this? I am running VS 17.5 Preview 2, I have updated my code base and rebuilt it but still cannot debug.

@MihaZupan
Copy link
Member

How did you go about starting the debug session?
Personally I've had the most luck adding Debugger.Launch directly into the source I'm interested in and then running the test suite via CLI. Does that work?

@BrunoBlanes
Copy link
Contributor Author

BrunoBlanes commented Dec 16, 2022

How did you go about starting the debug session?

I was just right clicking and selecting "Debug" in VS, but your method also gave me the same error:

image

The tests do indeed run, I just can't debug them. :/ Am I supposed to be using a specific version of .NET or VS?

PS C:\Users\bruno\runtime\src\libraries\System.Net.Http\tests\UnitTests> dotnet build /t:test /p:XunitMethodName=System.Net.Http.Unit.Tests.QPack.QPackDecoderTests.LiteralFieldWithoutNameReferece_ValueBrokenIntoSeparateBuffers
MSBuild version 17.4.0+18d5aef85 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
  Microsoft.Interop.SourceGeneration -> C:\Users\bruno\runtime\artifacts\bin\Microsoft.Interop.SourceGeneration\Debu
  g\netstandard2.0\Microsoft.Interop.SourceGeneration.dll
  LibraryImportGenerator -> C:\Users\bruno\runtime\artifacts\bin\LibraryImportGenerator\Debug\netstandard2.0\Microso
  ft.Interop.LibraryImportGenerator.dll
  TestUtilities -> C:\Users\bruno\runtime\artifacts\bin\TestUtilities\Debug\net6.0\TestUtilities.dll
  System.Net.Http.Unit.Tests -> C:\Users\bruno\runtime\artifacts\bin\System.Net.Http.Unit.Tests\Debug\net8.0-windows
  \System.Net.Http.Unit.Tests.dll
  ----- start Fri 12/16/2022 19:49:45.42 ===============  To repro directly: =======================================
  ==============
  pushd C:\Users\bruno\runtime\artifacts\bin\System.Net.Http.Unit.Tests\Debug\net8.0-windows\
  "C:\Users\bruno\runtime\artifacts\bin\testhost\net8.0-windows-Debug-x64\dotnet.exe" exec --runtimeconfig System.Ne
  t.Http.Unit.Tests.runtimeconfig.json --depsfile System.Net.Http.Unit.Tests.deps.json xunit.console.dll System.Net.
  Http.Unit.Tests.dll -xml testResults.xml -nologo -method System.Net.Http.Unit.Tests.QPack.QPackDecoderTests.Litera
  lFieldWithoutNameReferece_ValueBrokenIntoSeparateBuffers -notrait category=OuterLoop -notrait category=failing
  popd
  ===========================================================================================================
    Discovering: System.Net.Http.Unit.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Net.Http.Unit.Tests (found 1 of 915 test case)
    Starting:    System.Net.Http.Unit.Tests (parallel test collections = on, max threads = 4)
    Finished:    System.Net.Http.Unit.Tests
  === TEST EXECUTION SUMMARY ===
     System.Net.Http.Unit.Tests  Total: 1, Errors: 0, Failed: 0, Skipped: 0, Time: 29.397s
  ----- end Fri 12/16/2022 19:50:25.45 ----- exit code 0 ----------------------------------------------------------

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:01:35.99

@pedrobsaila
Copy link
Contributor

pedrobsaila commented Dec 26, 2022

@MihaZupan @BrunoBlanes I have the same issue (using VS 17.5.0 Preview 2.0). I tried also Debugger.Launch and got the same error.

System.Runtime.InteropServices.COMException (0x89710080): Unable to attach. A debug component is not installed.
   at Microsoft.VisualStudio.Shell.Interop.IVsDebuggerLaunchAsync.LaunchDebugTargetsAsync(UInt32 debugTargetCount, VsDebugTargetInfo4[] debugTargetArray, IVsDebuggerLaunchCompletionCallback completionCallback)
   at Microsoft.VisualStudio.TestWindow.ShellServices.DebuggerService.<>c__DisplayClass24_0.<<LaunchDebuggerAsync>b__0>d.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.VisualStudio.TestWindow.Logging.ILoggerExtensions.<CallWithCatchAsync>d__11`1.MoveNext()

Rollbacking to VS 17.5.0 Preview 1.0 made debugging works again.

@MihaZupan
Copy link
Member

Am I supposed to be using a specific version of .NET or VS?

You shouldn't need a preview version of VS. It seems to be working with 17.4.3 for example, but I too am running into issues with the current preview version.

Can you try using VS 17.5.0 Preview 1.0 as pedrobsaila suggested?

@dotnet/dnceng who would be the right contact to help here (the current preview of VS is failing to attach the debugger to a local build of dotnet/runtime)?

@dkurepa
Copy link
Member

dkurepa commented Dec 27, 2022

Hello, I'm not sure who'd be the best person to contact about this. @ilyas1974, could you recommend somebody?

@ilyas1974
Copy link

Larry Bynum would be the person to ask about issues with VS.

@danmoseley
Copy link
Member

@dotnet/dotnet-diag for that.

@tommcdon
Copy link
Member

Please try the workaround noted here. Although the error message does not match it may be that signature validation is the root cause:

Starting with Visual Studio 2022 version 17.5, Visual Studio will validate that the debugging libraries that shipped with the .NET Runtime are correctly signed before loading them. If they are unsigned, Visual Studio will show an error like:

Unable to attach to CoreCLR. Signature validation failed for a .NET Runtime Debugger library because the file is unsigned.

This error is expected if you are working with non-official releases of .NET (example: daily builds from https://github.com/dotnet/installer). See https://aka.ms/vs/unsigned-dotnet-debugger-lib for more information.

If the target process is using a .NET Runtime that is either from a daily build, or one that you built on your own computer, this error will show up. NOTE: This error should never happen for official builds of the .NET Runtime from Microsoft. So don’t disable the validation if you expect to be using a .NET Runtime supported by Microsoft.

There are three ways to configure Visual Studio to disable signature validation:

  1. The DOTNET_ROOT environment variable: if Visual Studio is started from a command prompt where DOTNET_ROOT is set, it will ignore unsigned .NET runtime debugger libraries which are under the DOTNET_ROOT directory.
  2. The VSDebugger_ValidateDotnetDebugLibSignatures environment variable: If you want to temporarily disable signature validation, run set VSDebugger_ValidateDotnetDebugLibSignatures=0 in a command prompt, and start Visual Studio (devenv.exe) from this command prompt.
  3. Set the ValidateDotnetDebugLibSignatures registry key: To disable signature validation on a more permanent bases, you can set the VS registry key to turn it off. To do so, open a Developer Command Prompt, and run Common7\IDE\VsRegEdit.exe set local HKCU Debugger\EngineSwitches ValidateDotnetDebugLibSignatures dword 0

@karelz
Copy link
Member

karelz commented Mar 7, 2023

We plan to look at it next week - @ManickaP

@ManickaP ManickaP marked this pull request as draft March 7, 2023 17:28
@ghost ghost closed this Apr 6, 2023
@ghost
Copy link

ghost commented Apr 6, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@rzikm rzikm reopened this Apr 11, 2023
@ManickaP ManickaP marked this pull request as ready for review April 13, 2023 13:27
@ManickaP
Copy link
Member

@MihaZupan I think this is ready for re-review.

cc @Tratcher @JamesNK

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Looks good. I admit I forgot some of the details around the decoder implementation, but your change in HPack looks similar enough to Qpack.

@ManickaP
Copy link
Member

@BrunoBlanes could you please resolve the CLA from #78862 (comment)? I cannot merge this without it.
The other option for me, would be to port the changes into a new PR of mine. I'd prefer not to as you're the original author here.

@BrunoBlanes
Copy link
Contributor Author

@dotnet-policy-service agree

@ManickaP ManickaP merged commit 0f0eba4 into dotnet:main Apr 19, 2023
ManickaP added a commit to ManickaP/runtime that referenced this pull request Apr 25, 2023
* Add test for literal field without name reference

* Fix header name buffer allocation

* Add more tests

* Unified QPackDecoderTest test files

* Fix variable name

* Fixed HPackDecoder and ported QPack tests

* Feedback

---------

Co-authored-by: ManickaP <mapichov@microsoft.com>
ManickaP added a commit to ManickaP/runtime that referenced this pull request Apr 25, 2023
* Add test for literal field without name reference

* Fix header name buffer allocation

* Add more tests

* Unified QPackDecoderTest test files

* Fix variable name

* Fixed HPackDecoder and ported QPack tests

* Feedback

---------

Co-authored-by: ManickaP <mapichov@microsoft.com>
ManickaP added a commit to ManickaP/runtime that referenced this pull request May 9, 2023
* Add test for literal field without name reference

* Fix header name buffer allocation

* Add more tests

* Unified QPackDecoderTest test files

* Fix variable name

* Fixed HPackDecoder and ported QPack tests

* Feedback

---------

Co-authored-by: ManickaP <mapichov@microsoft.com>
ManickaP added a commit to ManickaP/runtime that referenced this pull request May 9, 2023
* Add test for literal field without name reference

* Fix header name buffer allocation

* Add more tests

* Unified QPackDecoderTest test files

* Fix variable name

* Fixed HPackDecoder and ported QPack tests

* Feedback

---------

Co-authored-by: ManickaP <mapichov@microsoft.com>
MihaZupan pushed a commit that referenced this pull request May 15, 2023
* Add test for literal field without name reference

* Fix header name buffer allocation

* Add more tests

* Unified QPackDecoderTest test files

* Fix variable name

* Fixed HPackDecoder and ported QPack tests

* Feedback

---------

Co-authored-by: Bruno Blanes <bruno.blanes@outlook.com>
MihaZupan pushed a commit that referenced this pull request May 15, 2023
* Add test for literal field without name reference

* Fix header name buffer allocation

* Add more tests

* Unified QPackDecoderTest test files

* Fix variable name

* Fixed HPackDecoder and ported QPack tests

* Feedback

---------

Co-authored-by: Bruno Blanes <bruno.blanes@outlook.com>
@ghost ghost locked as resolved and limited conversation to collaborators May 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpRequestException: Destination is too short. (Parameter 'destination')