[WinHttpHandler] Move _cachedSendPinnedBuffer ownership to WinHttpRequestState

[antonfirsov]

A race between WinHttpRequestStream.WriteAsync and WinHttpRequestStream.Dispose can lead to Dispose unpinning the buffer forwarded to WinHttp, which can potentially lead to AccessViolationException:

runtime/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestStream.cs

Line 254 in 9fa1235

Marshal.UnsafeAddrOfPinnedArrayElement(buffer, offset),

This has been fixed in dotnet/corefx#6500 for WinHttpResponseStream, this PR is applying the same fix for WinHttpRequestStream.

[dotnet-policy-service]

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

[wfurt]

LGTM.

To answer https://github.com/dotnet/corefx/pull/6500/files#diff-8db7e154523836be1aa177c05ae0decb22c602791eabf11d01823c80ac9927d8R131
It seems like the WriteAsync has also similar logic to ReadAsync, right?

[antonfirsov]

It seems like the WriteAsync has also similar logic to ReadAsync, right?

Yes:

runtime/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestStream.cs

Lines 131 to 135 in 017593d

	if (_state.TcsInternalWriteDataToRequestStream != null &&
	!_state.TcsInternalWriteDataToRequestStream.Task.IsCompleted)
	{
	throw new InvalidOperationException(SR.net_http_no_concurrent_io_allowed);
	}

Note that the logic is prone to races in both streams, but that's beyond this PR.

[antonfirsov]

There are possibly related outerloop failures, this needs investigation.

[antonfirsov]

After a second look and more testing, the failures look unrelated:

• SendAsync_SlowServerRespondsAfterDefaultReceiveTimeout_ThrowsHttpRequestException is [Test Failure] System.Net.Http.WinHttpHandlerFunctional.Tests.WinHttpHandlerTest.SendAsync_SlowServerRespondsAfterDefaultReceiveTimeout_ThrowsHttpRequestException #96481
• InvalidCertificateServers_CertificateValidationDisabled_Succeeds also failed in an extensive run without changes, see [Test Only] WinHttp CI Test 1 #101939. The most likely root case is a connection issue to badssl.com.
• UseCallback_BadCertificate_ExpectedPolicyErrors also failed for SocketsHttpHandler in the run noted in [WinHttpHandler] Move _cachedSendPinnedBuffer ownership to WinHttpRequestState #101725 (comment), but did not fail in reruns. This again implies that the most likely root case is a connection issue.

I will do a couple of further outerloop reruns before merging this.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

All test failures are unrelated, merging.

[antonfirsov]

/backport to release/8.0-staging

[github-actions]

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/9034004390