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

[release/8.0] Fix pRequestInfo INVALID_POINTER_READ caused by GCs #50459

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot 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.

Backport of #50446 to release/8.0

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
@Tratcher Tratcher self-assigned this Aug 31, 2023
@Tratcher Tratcher added the Servicing-approved Shiproom has approved the issue label Aug 31, 2023
@ghost
Copy link

ghost commented Aug 31, 2023

Hi @github-actions[bot]. 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.

@Tratcher Tratcher added this to the 8.0-rc2 milestone Aug 31, 2023
@wtgodbe
Copy link
Member

wtgodbe commented Aug 31, 2023

@Tratcher can you get an approval on this? I can merge/enable auto-merge after that

@Tratcher
Copy link
Member

This was approved by tactics, but @adityamandaleeka can sign off locally.

@wtgodbe wtgodbe enabled auto-merge (squash) August 31, 2023 18:30
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Aug 31, 2023
@adityamandaleeka
Copy link
Member

Yup, approved

@wtgodbe wtgodbe merged commit 7f749ea into release/8.0 Aug 31, 2023
@wtgodbe wtgodbe deleted the backport/pr-50446-to-release/8.0 branch August 31, 2023 19:37
@ghost ghost modified the milestone: 8.0-rc2 Aug 31, 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 Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants