Skip to content

Fix GCHandle double-free race in WinHttpRequestState#125293

Merged
EgorBo merged 3 commits intodotnet:mainfrom
EgorBo:fix-gchandle
Mar 16, 2026
Merged

Fix GCHandle double-free race in WinHttpRequestState#125293
EgorBo merged 3 commits intodotnet:mainfrom
EgorBo:fix-gchandle

Conversation

@EgorBo
Copy link
Member

@EgorBo EgorBo commented Mar 7, 2026 •
edited
Loading

volatile bool _disposed doesn't make the following code atomic:

if (_disposed)
{
    return;
}
_disposed = true;

@EgorBo
Fix GCHandle double-free race in WinHttpRequestState and harden Pinne…
4baaa22 
…dBlob

- WinHttpRequestState.Dispose(): Replace volatile bool check-then-act
  with Interlocked.Exchange to eliminate TOCTOU race. Two code paths
  can reach Dispose (OnRequestHandleClosing callback and fallback
  disposal in StartRequest), and the non-atomic volatile bool allowed
  both threads to pass the guard simultaneously.

- PinnedBlob.Dispose(): Add IsAllocated guard before Free() and remove
  misleading comment that claimed Free() mutation prevents double-free
  (it only prevents it on the same struct instance, not on copies).

Co-authored-by: Copilot <[email protected]>
Copilot AI review requested due to automatic review settings March 7, 2026 18:39
@dotnet-policy-service
Copy link
Contributor

dotnet-policy-service bot commented Mar 7, 2026

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

Copilot AI reviewed Mar 7, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Addresses two potential double-free scenarios involving GCHandle.Free by making disposal idempotent under concurrency in WinHttpHandler, and by hardening a test utility’s Dispose implementation.

Changes:

  • Replace non-atomic volatile bool dispose guard with an Interlocked.Exchange-based guard in WinHttpRequestState.Dispose.
  • Update PinnedBlob.Dispose() (test utility) to only free the GCHandle when it’s allocated, and remove a misleading comment about copy-safety.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/libraries/System.Reflection.Metadata/tests/TestUtilities/PinnedBlob.cs Makes PinnedBlob.Dispose() robust against default/uninitialized instances by guarding GCHandle.Free() with IsAllocated.
src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestState.cs Prevents concurrent double-dispose/double-free by using an atomic dispose gate via Interlocked.Exchange.

@EgorBo EgorBo changed the title Fix GCHandle double-free race in WinHttpRequestState and harden PinnedBlob Fix GCHandle double-free race in WinHttpRequestState Mar 7, 2026
This was referenced Mar 7, 2026
Open
Open
@EgorBo EgorBo marked this pull request as ready for review March 7, 2026 23:08
Copilot AI review requested due to automatic review settings March 7, 2026 23:08
Copilot AI reviewed Mar 7, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@ManickaP
Copy link
Member

ManickaP commented Mar 9, 2026

Was this hit anywhere? Or is it just based on code observation?
From what I can see, we shouldn't be calling the Dispose neither more than once nor in-parallel ever. So if this was hit somewhere, we should probably look into it deeper.

Note that I have no qualms about making this change.

@EgorBo
Copy link
Member Author

EgorBo commented Mar 9, 2026

Was this hit anywhere? Or is it just based on code observation? From what I can see, we shouldn't be calling the Dispose neither more than once nor in-parallel ever. So if this was hit somewhere, we should probably look into it deeper.

Note that I have no qualms about making this change.

@ManickaP I can just remove the volatile then. The problem that the current change looks very suspicius. volatile always implies some multi-threading access is going on and the current code is incorrect. So it should be either removed if we don't need to protect from concurrent double-free or be Interlocked

Example:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

Lines 96 to 100 in 0d0e129

private void Dispose(bool disposing)
{
// Ensure we're only disposed once. Dispose could be called concurrently, for example,
// if the request and the response were running concurrently and both incurred an exception.
if (!Interlocked.Exchange(ref _disposed, true))

But it does look like volatile is not needed here, I just wasn't sure.

ManickaP
ManickaP reviewed Mar 9, 2026
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Feel free to merge this, I'm not against this change. I was just making sure we don't have any serious, unexpected problems we don't know about.

@build-analysis build-analysis bot mentioned this pull request Mar 16, 2026
Open
@EgorBo
Copy link
Member Author

EgorBo commented Mar 16, 2026

/ba-g deadletters on iOS.Device

@EgorBo EgorBo merged commit 28c5a4d into dotnet:main Mar 16, 2026
85 of 90 checks passed
@EgorBo EgorBo deleted the fix-gchandle branch March 16, 2026 23:14
@dotnet-maestro dotnet-maestro bot mentioned this pull request Mar 17, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@ManickaP ManickaP ManickaP approved these changes

@AntonLapounov AntonLapounov AntonLapounov approved these changes

@MihaZupan MihaZupan MihaZupan approved these changes

Assignees

@EgorBo EgorBo

Labels

area-System.Net.Http

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@EgorBo @ManickaP @AntonLapounov @MihaZupan