Skip to content

Conversation

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

@ghost ghost 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.

5 participants