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 pRequestInfo INVALID_POINTER_READ caused by GCs (v7 backport) #50448

Conversation

NGloreous
Copy link
Contributor

@NGloreous NGloreous commented Aug 31, 2023

Fix pRequestInfo INVALID_POINTER_READ caused by GCs

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Backports #50446 to v7.

Description

When using the IHttpSysRequestInfoFeature it's possible to hit invalid pointer read errors which causes a process crash. It's also possible to hit other issue caused by the invalid pointer. The error occurs when a GC moves to memory of the request's _backingBuffer before trying to access the RequestInfo property of IHttpSysRequestInfoFeature.

The fix is to adjust the pRequestInfo pointer similar to how other pointers in this class are adjusted.

Fixes #50445

Customer Impact

It's not possible to use the IHttpSysRequestInfoFeature without eventually crashing the process due to the INVALID_POINTER_READ.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

The change is very small an in-line with how pointers are used/updated within NativeRequestContext.

Verification

  • Manual (required)
  • Automated

I haven't been able to reproduce the crash locally, but I was able to confirm this is the root caused from a crash dump. I did verify this change did not regress this feature.

Packaging changes reviewed?

  • Yes
  • No
  • N/A

When servicing release/2.1

  • Make necessary changes in eng/PatchConfig.props

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Aug 31, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 31, 2023
@ghost
Copy link

ghost commented Aug 31, 2023

Thanks for your PR, @NGloreous. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@ghost ghost added this to the 7.0.x milestone Aug 31, 2023
@ghost
Copy link

ghost commented Aug 31, 2023

Hi @NGloreous. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@NGloreous NGloreous changed the title Fix pRequestInfo INVALID_POINTER_READ caused by GCs Fix pRequestInfo INVALID_POINTER_READ caused by GCs (v7 backport) Aug 31, 2023
@Tratcher Tratcher self-assigned this Aug 31, 2023
@Tratcher Tratcher added the Servicing-consider Shiproom approval is required for the issue label Aug 31, 2023
@ghost
Copy link

ghost commented Aug 31, 2023

Hi @NGloreous. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@Tratcher Tratcher added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Aug 31, 2023
@ghost
Copy link

ghost commented Aug 31, 2023

Hi @NGloreous. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@wtgodbe wtgodbe merged commit 0a51ea1 into dotnet:release/7.0 Sep 1, 2023
@ghost ghost modified the milestones: 7.0.x, 7.0.12 Sep 1, 2023
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Sep 1, 2023
@rbhanda rbhanda modified the milestones: 7.0.12, 7.0.14 Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member Servicing-approved Shiproom has approved the issue
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants