You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix Network.framework TLS handshake framing, SNI and EOF handling on macOS - #134260
Work in progress, but at a coherent checkpoint: the full System.Net.Security suite now passes under System.Net.Security.UseNetworkFramework on macOS. Posting it now for early feedback on direction rather than as a finished change.
The main problem: over-reading past the handshake
The bug that motivated this is the one most likely to bite real callers, and it is not obvious from the symptom.
The Network.framework transport pump always read in bulk. Network.framework never asks for a specific number of bytes, so the pump speculatively reads whatever the inner stream has and feeds it in. That is fine while the connection carries nothing but TLS — but it is not true of every caller.
SqlClient tunnels the TLS handshake inside TDS packets and stops that encapsulation the moment AuthenticateAsClient returns. So the bytes immediately after the final handshake record are framed differently from the ones before it. A bulk read that ran past the end of the handshake consumed the first post-handshake record and interpreted a raw TLS record as a TDS header — surfacing as TLS error -9836, far from the actual mistake.
The fix is to make handshake reads frame precise: consume exactly one TLS record per iteration while the handshake is in flight, cancel the speculative read as soon as the handshake completes, and then leave the transport untouched until the application starts its own I/O — by which point it has finished re-framing the stream. Bulk reads resume afterwards, where they are safe.
Also fixed
SNI was never reported to servers. Network.framework parses the ClientHello internally, so unlike the other PALs the managed server cannot recover the host name from the transport, and SslStream.TargetHostName stayed empty. It is now read from connection metadata and decoded through the existing TlsFrameHelper path, so a server sees the same Unicode host the client passed. Client-side TargetHostName is deliberately left alone, since the wire form is punycode.
Truncated TLS records looked like a clean EOF. Cancelling the connection on transport EOF is what lets Dispose complete, but it also converted a mid-record truncation into a graceful close, so a caller read 0 instead of failing. The pump now tracks record boundaries and raises IOException only when the stream ends mid-record.
A use-after-free that aborted the process.Dispose completed the transport read source itself, letting the read loop finish while Network.framework still owned a framer delivery; the GCHandle was freed and the native completion then resolved a dead handle. This reproduced as a SIGABRT partway through a full test run, not as a test failure.
An unbounded strcpy for the negotiated ALPN was replaced with a length-checked copy, now shared with the server-name output.
Caveats and documented divergences
Two behaviours are documented rather than emulated, so they need a explicit decision before the switch is flipped:
ClientCertificateRequired is strict. .NET treats it as "request a certificate and let RemoteCertificateValidationCallback decide", so a callback may accept a client that sent none. Network.framework enforces the requirement itself and aborts the handshake before managed validation runs. sec_protocol_options_set_peer_authentication_optional expresses exactly the semantics .NET wants, but it is declared API_UNAVAILABLE(macos, ios, watchos, tvos) and, per its own header, is disregarded whenever peer_authentication_required is set. mTLS works normally whenever the client actually presents a certificate; only the accept-the-absence case diverges. Documented at the call site and pinned by NetworkFramework_MissingClientCertificate_FailsHandshake; the two affected rows of CertificateSelectionCallback_DelayedCertificate_OK are skipped.
Inner-stream reads happen on the transport pump, not the caller's thread, so tests asserting synchronous inner reads do not apply and are skipped for this PAL.
I considered mapping the first case to PlatformNotSupportedException, but it cannot be detected from configuration alone — only from the outcome — so it would have meant inferring intent by invoking the user's validation callback after the handshake had already failed. Documenting it is the smaller lie.
Validation
Full System.Net.Security functional suite on macOS arm64, both PALs:
PAL
Total
Failed
Skipped
SecureTransport (default)
5045
0
30
Network.framework
4782
0
89
The TLS-over-TDS scenario is additionally covered by a standalone repro that asserts no non-TLS framing is consumed; it passes on both PALs and over 15 consecutive runs.
Skips are higher under Network.framework because of the documented divergences above and pre-existing unsupported configurations that fall back to SecureTransport.
Still to do
Decide whether the ClientCertificateRequired divergence is acceptable or needs an API/behavioural answer before enabling by default.
A sweeping review pass is planned before flipping the default.
No breaking-change doc yet; that is only warranted if this becomes the default.
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.
ArrayPool<byte>.Shared.Rent(MaxTlsRecordSize) is allowed to return an array larger than MaxTlsRecordSize, and this Memory<byte> exposes the entire rented array. As a result, a header advertising a record larger than the RFC limit can pass the header.Length > buffer.Length check (for example, in a larger pool bucket) and consume more than the documented maximum. Limit the memory passed to ReadSingleTlsRecordAsync to MaxTlsRecordSize so the bound is enforced.
Session queue retain leaks when setup fails before framer startup
The explicit dispatch_retain(sessionQueue) is balanced only by the cleanup handler installed from framer_start. Any connection/listener setup failure before a framer starts (for example, listener creation/readiness or inbound-delivery failure) releases the local queue reference but never this retain, leaking the per-session queue. Tie the ownership to a cleanup path that also covers setup failures, or explicitly release it on every pre-start abort path.
The explicit dispatch_retain is only balanced by the cleanup handler installed in framer_start. If nw_connection_create or nw_listener_create fails, no framer starts and that handler is never installed; the existing dispatch_release(sessionQueue) in those failure paths only balances the queue created at lines 401/455, leaving this extra retain leaked. Add an error-path rollback for the options retain (and cover any connection-failure path before framer_start).
The reason will be displayed to describe this comment to others. Learn more.
It does not seem right to do sync-over-async, which streams reject synchronous operations? If it is some of our internal tests then we should get the exception for this configuration.
The reason will be displayed to describe this comment to others. Learn more.
Fair challenge. I tried to answer it empirically rather than from memory, and the result is more interesting than I expected.
Why it blocks at all: this is the native output handler. Network.framework calls it on its own queue with a void signature and no continuation, so the bytes have to reach the transport before it returns — something has to block. The choice is only which blocking call.
Which streams reject sync: I reverted it to plain TransportStream.Write(data) and ran the suite to find out. No test threw NotSupportedException, so I cannot point at a concrete stream in-tree, and I should not have implied there was a specific one. The justification is the general contract — SslStream wraps a caller-supplied stream, and async-only streams are legal — not an observed failure.
But the probe did not come back clean. With the synchronous Write the run aborted:
Process terminated. Assertion failed.
Expected thisContext to be non-null
at SafeDeleteNwContext.<FillAppDataBufferAsync>g__CompletionCallback|50_0(...)
That is a receive completion resolving a freed GCHandle — a latent lifetime bug in a different callback from the send one fixed in 2d98ec5. Blocking the NW queue in the output handler evidently shifts timing enough to expose it. It reproduces only under the sync variant; the async form has run the full suite repeatedly without it.
So I have left the async form in place, and I am not claiming it is the principled choice — sync-over-async here is a smell, and you are right to flag it. The principled fix is to stop blocking this queue at all and let the framer drive the write asynchronously, which is a larger change than this PR should carry. I would rather track both that and the receive-callback lifetime as follow-ups than paper over them here. Happy to file those, or fold them in if you would prefer them fixed before this merges.
Note
Reply drafted with AI assistance (GitHub Copilot).
dispatch_retain(sessionQueue) has a matching release only in the cleanup handler installed from framer_start. If connection creation, listener setup, or immediate cancellation fails before Network.framework invokes that start handler, the cleanup handler is never installed and this per-session queue retain leaks. Release the extra retain on every pre-start failure path or use an ownership callback guaranteed to run even when the framer never starts.
The extra dispatch_retain(sessionQueue) is balanced only by the framer cleanup handler, but that handler is never installed or run when connection/listener creation fails or any of the later server bootstrap early returns fire. Those failure paths therefore leak one queue retain per failed setup attempt. Keep the queue ownership in a scope that is released on every failure path, or explicitly balance this retain before each early return.
Status note ahead of another review pass — what changed since the last one, and how it was validated locally.
Changes since the previous review
commit
what
f6599625
Review feedback: ReadAtLeastAsync, out parameter, DecodeString, SNI test reuse
61b7b7b
Fix to that test reuse — it was not exercising the new PAL at all (below)
1f4a9af
Document why the handshake read is speculative and the contract it implies
f32a703
Close an admission race between an application send and disposal
61b7b7b is worth a second look
Re-using SslStream_ClientSendsSNIServerReceives_Ok for the Network.framework coverage — which was the right instinct and the previous suggestion — silently removed the coverage. That test supplies the certificate through ServerCertificateSelectionCallback, and ShouldUseNetworkFramework requires a CertificateContext up front for a server, so the server quietly fell back to SecureTransport.
Caught by deleting the SNI propagation entirely and observing the test still pass. The server now sets ServerCertificate directly, verified both ways: 5/5 pass with the propagation, 4/5 fail without it (the empty-host case passes either way, since expected and actual are both "").
Local validation
Full functional suite, both PALs, on macOS arm64:
PAL
Total
Failed
Skipped
SecureTransport (default)
5048
0
30
Network.framework
4785
0
89
Stability soak. Because several problems here were timing dependent — a use-after-free that aborted a whole test run, and a hang — the PAL was also soaked with a standalone TLS-over-TDS program modelling SqlClient's re-framing transport, run 8-way parallel to keep the machine contended (load average ~70 on 16 cores):
55,040 runs, 0 failures on f32a703, still running.
A further 53,528 runs on earlier commits produced a single failure: a 15s timeout under artificial 8× load with Unexpected headers: 0, i.e. no misframing. It also predates f32a703.
No framing error in ~108,000 runs.
That is stability on one machine, not a claim about CI hardware, which is slower and more contended.
One thing to decide, not to review
1f4a9af documents rather than fixes a limitation: a transport that re-frames after the handshake and ignores read cancellation is not supported. Measured:
transport
honors cancellation
ignores cancellation
no re-framing
pass
pass
re-frames
pass
fails
Ignoring cancellation alone is harmless; only the combination breaks. SslOverTdsStream re-frames but forwards cancellation to the socket, so the motivating case is supported.
This is a regression against legacy, which is pull-based and has no speculative read. A deterministic fix needs a "needs more input" signal that Network.framework does not expose — nw_framer_set_input_handler is edge triggered on data arriving from below, the framer here sits above a socket that never carries traffic, and the transport cannot be disabled. Details in the thread on SafeDeleteNwContext.cs.
Worth an explicit decision before UseNetworkFramework becomes the default, rather than at flip time.
Note
This comment was generated with AI assistance (GitHub Copilot).
The Network.framework PAL could lose inbound data, read past the end of the
handshake, and hang instead of reporting failures. Four related defects:
Inbound data was discarded when it arrived before the framer started.
WriteInboundWireDataAsync dropped the buffer whenever _framerHandle was still
null. The transport read loop starts with the connection, but the framer only
appears when Network.framework invokes the start handler, so a peer's first
flight could be thrown away - reliably so over an in-memory transport, where
the write is visible immediately. TLS never retransmits, so the handshake then
waited forever. Delivery now waits for the framer instead of dropping.
The read loop consumed bytes past the final handshake record. It always read in
bulk, so it could take application data that the peer had already re-framed -
SqlClient's TLS-over-TDS stream leaves TDS encapsulation only after
AuthenticateAsClient returns, and the trailing read consumed a raw TLS record as
a TDS header (reported as TLS error -9836). Handshake reads are now frame
precise, the speculative read is cancelled as soon as the handshake completes,
and the loop then idles until the application starts its own I/O.
Outbound writes used the transport's synchronous API from a Network.framework
queue, which fails on streams that reject synchronous operations, and the
failure only ever reached a pending application write. During the handshake
there is none, so the error was swallowed and the connection hung. Writes now
use the asynchronous API and failures fault the handshake.
Disposal released the application receive buffer while a native receive
completion could still write into it, and an exception escaping that callback
terminated the process. The buffer is now released only after the connection
reports cancellation, and the callback no longer lets exceptions escape.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 021e2d41-d78f-4c87-96ff-ce25bbe7be9c
SslServerAuthenticationOptions.ClientCertificateRequired was silently ignored:
nothing ever called sec_protocol_options_set_peer_authentication_required, so
the server never sent a CertificateRequest. The client therefore sent no
certificate, the verify block never produced a peer certificate, and the
callback saw a null certificate with a null chain. An application asking for
client certificates got an unauthenticated connection instead of an error.
The requirement is now plumbed from the managed options into the connection.
Network.framework has no API for advertising a custom CA list in the
CertificateRequest, so a server configured with SslCertificateTrust now stays on
SecureTransport instead of quietly dropping the caller's trust list, using the
fallback that already exists for configurations the new PAL cannot satisfy.
Also stop asserting that framer options are available. They are gone once the
framer is being torn down, which is exactly when the cleanup handler reads them,
so the assert fired during ordinary teardown in debug builds.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 021e2d41-d78f-4c87-96ff-ce25bbe7be9c
…nection
SslStream.ShutdownAsync builds a shutdown token by calling ApplyShutdownToken and
then GenerateToken. The first already handles a Network Framework context by
cancelling the connection, but the second reached the SecureTransport-only
handshake path, whose debug assertion that the context is a SafeDeleteSslContext
then terminated the process instead of failing a single test.
Network Framework emits close_notify itself once the connection is cancelled, so
there is no token for SslStream to send. Report success for that context type.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 021e2d41-d78f-4c87-96ff-ce25bbe7be9c
Three unrelated defects in the Network.framework PAL, plus the test changes
that pin the behaviour.
The server never saw the requested host name. Network.framework parses the
ClientHello itself, so the managed server cannot recover SNI from the transport
stream the way the other PALs do, and SslStream.TargetHostName stayed empty.
The negotiated name is now read back from the connection metadata and applied
to the server context. It arrives as ASCII/punycode and is decoded through the
existing TlsFrameHelper path, so a server observes the same Unicode host name
the client passed. Only the server side is updated; a client keeps the caller's
original string. AppleCryptoNative_GetConnectionInfo gained the extra output,
and the unbounded strcpy it used for the negotiated ALPN was replaced with a
length-checked copy shared by both strings.
A transport EOF in the middle of a TLS record looked like a clean shutdown.
Cancelling the connection on EOF is what lets Dispose complete, but it also
turned a truncated record into a graceful close, so a caller read 0 instead of
failing. The transport pump now tracks encrypted record boundaries and faults
the pending read with IOException when the stream ends mid-record, while an EOF
on a boundary keeps the existing cancellation path. Only the record length is
retained, so no buffering is added.
Disposal could free the callback context while Network.framework still owned a
framer delivery. Dispose completed the transport read source itself, which let
the read loop finish early; the GCHandle was then released and the native
completion resolved a freed handle, aborting the process. The delivery is left
to be completed by its callback, which is what keeps the handle alive.
Two behaviours are documented rather than emulated. Network.framework enforces
ClientCertificateRequired itself and fails the handshake when the client sends
no certificate, so RemoteCertificateValidationCallback never gets to accept the
absence: sec_protocol_options_set_peer_authentication_optional expresses what
.NET wants but is API_UNAVAILABLE on macOS, and is disregarded when
peer_authentication_required is set. Reads of the inner stream also happen on
the transport pump rather than the caller's thread, so tests asserting
synchronous inner reads do not apply to this PAL.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 80f9832a-76b3-4767-aa82-58ab20582da2
… leak
Three issues found by automated review of the Network.framework PAL.
AppleCryptoNative_NwFramerDeliverInput returns -1 without invoking the
completion callback when nw_framer_message_create fails, which happens when the
connection is failing or being cancelled. The managed caller discarded that
return and awaited the completion, so the transport pump and a later Dispose
could hang on exactly the race this path exists to handle. The status is now
checked and the pending source faulted.
The transport pump treated any completed handshake as a successful one. The
handshake source completes with an Exception value on failure and is cancelled
when authentication is cancelled, so a delivery returning after either of those
moved the pump into the post-handshake state and parked it on application I/O
that would never start. The transition now requires successful completion and
the pump stops otherwise.
The framer cleanup handler looked the session queue up through the framer
options to balance the retain taken when the options were built. Options are
unavailable once teardown has started - the lookup is explicitly tolerant of
that - so the release was skipped exactly when cleanup ran, leaking a dispatch
queue per connection. The queue is now captured while the options are still
readable and released from the captured block.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 80f9832a-76b3-4767-aa82-58ab20582da2
Dispose released the GCHandle that native callbacks resolve once the transport
read loop had stopped, but an application send is not covered by that wait.
AppleCryptoNative_NwConnectionSend's completion block and the framer output
write both resolve the handle, so a write still in flight when the stream is
disposed can resolve a freed handle. Because GCHandle slots are reused, that
does not reliably fault: the callback can resolve to an unrelated connection
created afterwards and corrupt it.
An in-flight send is now published before the native call is issued and is
completed only after both callbacks have been observed, and Dispose waits for
it before releasing the handle, deferring the release when it does not settle
in the bounded wait.
The wait deliberately runs after Dispose faults the pending write completion.
A write parked on that source is unblocked by Dispose itself, so waiting any
earlier would deadlock the two against each other.
The accompanying test covers the shutdown ordering but does not reproduce the
use-after-free: on loopback the send completion has already run by the time
Dispose is reached, so it passes with and without the wait. It is kept as a
regression guard against Dispose hanging or crashing on a racing write.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 80f9832a-76b3-4767-aa82-58ab20582da2
Replace the manual read loops in ReadSingleTlsRecordAsync and the framing test
stream with Stream.ReadAtLeastAsync(throwOnEndOfStream: false), which expresses
"fill this buffer, tell me if the stream ended first" directly.
Return the negotiated server name from UpdateSslConnectionInfo as an out
parameter. Every other platform's overload returns void, so the OSX one is
easier to read the same way.
Drop the DecodeSni wrapper and make TlsFrameHelper.DecodeString internal.
The wrapper only renamed an existing method for a single caller.
Re-use SslStream_ClientSendsSNIServerReceives_Ok and its HostNameData for the
Network.framework SNI coverage instead of duplicating a narrower copy of it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 80f9832a-76b3-4767-aa82-58ab20582da2
The previous commit re-used SslStream_ClientSendsSNIServerReceives_Ok for the
Network.framework coverage, but that test configures the server with a
ServerCertificateSelectionCallback and no ServerCertificate. ShouldUseNetworkFramework
requires a CertificateContext up front for a server, so the server fell back to
SecureTransport and the test passed with the SNI propagation removed entirely.
Supply ServerCertificate directly so the server really runs on the new PAL, and
keep re-using HostNameData for the host names, plus the empty name a client that
sends no SNI produces. Verified by removing the propagation again: 4 of the 5
cases fail without it and all pass with it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 80f9832a-76b3-4767-aa82-58ab20582da2
The handshake read loop depends on cancellation to abandon its speculative read
before the application re-frames the transport, which is easy to mistake for a
detail that could simply be removed.
Record why it cannot: the other PALs read on demand because the handshake tells
them when it needs more input, and Network.framework has no equivalent signal.
A framer input handler is edge triggered on data arriving from the layer below
rather than on TLS wanting bytes, and the transport below the framer cannot be
disabled, so there is nothing to wait on.
Also record the resulting contract. A transport that re-frames after the
handshake has to honour read cancellation, which SslOverTdsStream does by
forwarding the token to the socket. Ignoring cancellation is harmless on its own,
because the late read then just returns an ordinary TLS record; it is only the
combination with re-framing that consumes application bytes under the old
framing, and that combination is not supported.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 80f9832a-76b3-4767-aa82-58ab20582da2
_pendingSendCompletion was published as a plain field and read by Dispose from
another thread. A send could be about to call into native code while Dispose
read the field as null, concluded nothing was outstanding, and freed the
GCHandle that the send completion then resolves.
Publishing the field earlier narrowed the window but could not close it: the
check and the native call have to be atomic with respect to disposal, which no
amount of volatile reads gives on its own.
Both now happen under a lock that Dispose also takes to set _disposed and to
snapshot any send already admitted. A send that wins the race is waited for; one
that loses is refused with ObjectDisposedException rather than reaching native
code after the handle is gone.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 80f9832a-76b3-4767-aa82-58ab20582da2
…ncellation
A transport EOF during the handshake left _handshakeCompletionSource untouched.
The loop cancelled the connection and stopped reading, so the handshake only
failed once Network.framework delivered ConnectionCancelled and ConnectionClosed
completed the source. That callback is not prompt: under load it can arrive well
after the peer is known to be gone, so AuthenticateAsClient/AuthenticateAsServer
stayed pending long past the point the other PALs report failure, which read EOF
from the transport and fail on the spot.
The handshake is now failed directly when the transport ends while it is still in
progress. The connection is still cancelled, so the lifecycle is unchanged; only
the latency of reporting the failure is.
Found through an intermittent failure of
SslStreamStreamToStreamTest_SyncParameters.SslStream_StreamToStream_Handshake_DisposeClient_Throws
under full suite load, where the disposed client failed immediately but the server
did not fail inside the three second window the test helper allows for collecting
a second exception, so the test saw one exception instead of two.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 80f9832a-76b3-4767-aa82-58ab20582da2
The reason will be displayed to describe this comment to others. Learn more.
TL;DR: My local functional test run with this PR applied reported no failures. Four points still need review. Two are described inline, and two from Copilot's summaries are listed below.
I left two inline findings at 029fa46: partial-record bytes lost on handshake-read cancellation, and an untracked native receive after EOF.
Two points from Copilot's review summaries also remain at this head:
The extra dispatch_retain(sessionQueue) is released only by framer cleanup, so setup failure before the framer starts leaks that retain.
ReadSingleTlsRecordAsync bounds header.Length against the whole rented array, which is 32 KB for this request, rather than MaxTlsRecordSize.
I tested on macOS 27.0.1 (arm64), using main b3c0988 with #136214 and this PR applied. #136214 changes only TlsSession and its tests. I ran the System.Net.Security functional tests with DOTNET_SYSTEM_NET_SECURITY_USENETWORKFRAMEWORK=1, excluding tests with OuterLoop or failing traits. The run reported 4,813 total, 0 failed, 86 skipped. TlsSessionTests.ClientSession_AgainstSslStreamServer_HandshakeAndPingPong_Succeeds times out at its 30-second handshake wait on main and with #136214 alone, and passes with this PR applied. With the switch off, TlsSessionTests pass on both main and the combined tree.
The reason will be displayed to describe this comment to others. Learn more.
If handshake completion cancels ReadSingleTlsRecordAsync after it has consumed part of a record, this catch discards those bytes. Both ReadAtLeastAsync calls use the handshake token; cancellation after the header, or after a partial header/body read, loses the consumed prefix. After the application starts I/O, bulk reads resume mid-record.
This can happen with cancellation honored and no re-framing. In the TLS 1.3 client window described in your trace in #134260 (comment), the speculative read can consume a prefix of the next record before NW reports ready and cancels it. A server may send application data immediately after its Finished, so this is distinct from the documented re-framing-plus-ignored-cancellation limitation.
The transition needs to retain partial-read progress across the pause. Finishing a started record with only _shutdownCts.Token would need additional synchronization: otherwise the read could remain active when authentication returns and the caller changes transport framing.
The reason will be displayed to describe this comment to others. Learn more.
After a read returns 0, _appReceiveBufferTcs is final, but the next read still calls NwConnectionReceive. TryGetValueTask returns the completed source, so this second read also returns 0 immediately and clears _pendingAppReceiveBufferFillTask, leaving the native callback untracked.
The sequence is EOF on a record boundary, a read returning 0, then another read after the pump and connection-cancel notification have completed. If Dispose runs before the second receive callback, its waits are already satisfied and it can free _thisHandle before the callback calls ResolveThisHandle. Returning 0 without another native receive once the source is final would prevent this sequence. I found this by reading the code and have not reproduced it.
The final-source/native-call pattern predates this PR. This is a code-derived path related to the lifetime issue in #134260 (comment); that reply reports a crash only with synchronous Write and proposes tracking the lifetime issue as a follow-up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
rzikm
left a comment
pcshrosbree
left a comment
contributes to #1979
Work in progress, but at a coherent checkpoint: the full
System.Net.Securitysuite now passes underSystem.Net.Security.UseNetworkFrameworkon macOS. Posting it now for early feedback on direction rather than as a finished change.The main problem: over-reading past the handshake
The bug that motivated this is the one most likely to bite real callers, and it is not obvious from the symptom.
The Network.framework transport pump always read in bulk. Network.framework never asks for a specific number of bytes, so the pump speculatively reads whatever the inner stream has and feeds it in. That is fine while the connection carries nothing but TLS — but it is not true of every caller.
SqlClienttunnels the TLS handshake inside TDS packets and stops that encapsulation the momentAuthenticateAsClientreturns. So the bytes immediately after the final handshake record are framed differently from the ones before it. A bulk read that ran past the end of the handshake consumed the first post-handshake record and interpreted a raw TLS record as a TDS header — surfacing as TLS error-9836, far from the actual mistake.The fix is to make handshake reads frame precise: consume exactly one TLS record per iteration while the handshake is in flight, cancel the speculative read as soon as the handshake completes, and then leave the transport untouched until the application starts its own I/O — by which point it has finished re-framing the stream. Bulk reads resume afterwards, where they are safe.
Also fixed
SslStream.TargetHostNamestayed empty. It is now read from connection metadata and decoded through the existingTlsFrameHelperpath, so a server sees the same Unicode host the client passed. Client-sideTargetHostNameis deliberately left alone, since the wire form is punycode.Disposecomplete, but it also converted a mid-record truncation into a graceful close, so a caller read0instead of failing. The pump now tracks record boundaries and raisesIOExceptiononly when the stream ends mid-record.Disposecompleted the transport read source itself, letting the read loop finish while Network.framework still owned a framer delivery; theGCHandlewas freed and the native completion then resolved a dead handle. This reproduced as aSIGABRTpartway through a full test run, not as a test failure.strcpyfor the negotiated ALPN was replaced with a length-checked copy, now shared with the server-name output.Caveats and documented divergences
Two behaviours are documented rather than emulated, so they need a explicit decision before the switch is flipped:
ClientCertificateRequiredis strict. .NET treats it as "request a certificate and letRemoteCertificateValidationCallbackdecide", so a callback may accept a client that sent none. Network.framework enforces the requirement itself and aborts the handshake before managed validation runs.sec_protocol_options_set_peer_authentication_optionalexpresses exactly the semantics .NET wants, but it is declaredAPI_UNAVAILABLE(macos, ios, watchos, tvos)and, per its own header, is disregarded wheneverpeer_authentication_requiredis set. mTLS works normally whenever the client actually presents a certificate; only the accept-the-absence case diverges. Documented at the call site and pinned byNetworkFramework_MissingClientCertificate_FailsHandshake; the two affected rows ofCertificateSelectionCallback_DelayedCertificate_OKare skipped.I considered mapping the first case to
PlatformNotSupportedException, but it cannot be detected from configuration alone — only from the outcome — so it would have meant inferring intent by invoking the user's validation callback after the handshake had already failed. Documenting it is the smaller lie.Validation
Full
System.Net.Securityfunctional suite on macOS arm64, both PALs:The TLS-over-TDS scenario is additionally covered by a standalone repro that asserts no non-TLS framing is consumed; it passes on both PALs and over 15 consecutive runs.
Skips are higher under Network.framework because of the documented divergences above and pre-existing unsupported configurations that fall back to SecureTransport.
Still to do
ClientCertificateRequireddivergence is acceptable or needs an API/behavioural answer before enabling by default.