From 25f0fb639f432c3ed8b0e0769afe6f8a745f2e18 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 26 Aug 2026 11:04:42 +0200 Subject: [PATCH 01/10] Invalidate SmtpClient connection when Host or Port changes SmtpClient caches a live SmtpConnection in its transport and reuses it across Send calls, but the Host and Port setters only cleared the legacy _servicePoint and never dropped the cached connection. As a result, changing Host or Port between sends kept delivering mail to the original server. Release the cached connection when Host or Port actually changes so the next send establishes a fresh connection to the new target. Add regression tests across the sync Send, SendAsync, and SendMailAsync paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d288b6ab-b489-4bb8-afb1-5552f3bae55d --- .../src/System/Net/Mail/SmtpClient.cs | 6 +++ .../Functional/SmtpClientConnectionTest.cs | 39 +++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs index a87555145fbae8..0eb2418e0263a2 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs @@ -164,6 +164,9 @@ public string? Host { _host = value; _servicePoint = null; + // The cached connection targets the previous host, so release it + // to force a new connection to be established on the next send. + _transport.ReleaseConnection(); } } } @@ -187,6 +190,9 @@ public int Port { _port = value; _servicePoint = null; + // The cached connection targets the previous port, so release it + // to force a new connection to be established on the next send. + _transport.ReleaseConnection(); } } } diff --git a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs index 5dfa223a8ca8c2..86d89c50fdd743 100644 --- a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs +++ b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs @@ -84,6 +84,45 @@ public async Task EHelloNotRecognized_RestartWithHello() await SendMail(new MailMessage("mono@novell.com", "everyone@novell.com", "introduction", "hello")); Assert.True(helloReceived, "HELO command was not received."); } + + [Fact] + public async Task ChangingPort_DoesNotReuseConnectionToPreviousServer() + { + using LoopbackSmtpServer server2 = new LoopbackSmtpServer(Output); + + await SendMail(new MailMessage("first@example.com", "everyone@novell.com", "introduction", "hello")); + Assert.Equal("", Server.MailFrom); + + // Point the client at a different server. The cached connection to the original + // server must be dropped so the next message is delivered to the new server. + Smtp.Port = server2.Port; + + await SendMail(new MailMessage("second@example.com", "everyone@novell.com", "introduction", "hello")); + + Assert.Equal("", server2.MailFrom); + Assert.Equal(1, server2.ConnectionCount); + + // The original server must not have received the second message. + Assert.Equal("", Server.MailFrom); + } + + [Fact] + public async Task ChangingHost_EstablishesNewConnection() + { + Server.ReceiveMultipleConnections = true; + + await SendMail(new MailMessage("first@example.com", "everyone@novell.com", "introduction", "hello")); + Assert.Equal(1, Server.ConnectionCount); + Assert.Equal("", Server.MailFrom); + + // Change the host to another value that still resolves to the loopback server. + // The cached connection must be dropped and a new one established. + Smtp.Host = "127.0.0.1"; + + await SendMail(new MailMessage("second@example.com", "everyone@novell.com", "introduction", "hello")); + Assert.Equal(2, Server.ConnectionCount); + Assert.Equal("", Server.MailFrom); + } } public class SmtpClientConnectionTest_Send : SmtpClientConnectionTest From 4b2be72caa27357a7985f09750f7c6db3852b5d6 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 26 Aug 2026 11:57:08 +0200 Subject: [PATCH 02/10] Invalidate cached connection for all connection-affecting properties Extend the SmtpClient connection-reuse fix beyond Host/Port to cover the other properties that change how a connection is established: Credentials, UseDefaultCredentials, EnableSsl, and TargetName. Changing any of these now invalidates the cached connection so the next send establishes a fresh one. Move the potentially blocking connection shutdown off the property setters: setters only mark the transport stale (InvalidateCachedConnection), and the graceful close of the old connection happens lazily on the send path inside GetConnectionAsync. IsConnected reports false while stale so EnsureConnection falls through to reconnect. Generalize the host regression test into a theory covering Host, Credentials, and TargetName, and add a TLS test verifying that enabling EnableSsl after an initial plaintext send establishes a new encrypted connection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d288b6ab-b489-4bb8-afb1-5552f3bae55d --- .../src/System/Net/Mail/SmtpClient.cs | 24 ++++++++++---- .../src/System/Net/Mail/SmtpTransport.cs | 30 +++++++++++++++-- .../Functional/SmtpClientConnectionTest.cs | 32 ++++++++++++++++--- .../tests/Functional/SmtpClientTlsTest.cs | 21 ++++++++++++ 4 files changed, 92 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs index 0eb2418e0263a2..b73959054262a5 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs @@ -164,9 +164,9 @@ public string? Host { _host = value; _servicePoint = null; - // The cached connection targets the previous host, so release it - // to force a new connection to be established on the next send. - _transport.ReleaseConnection(); + // The cached connection targets the previous host, so invalidate it to force + // a new connection to be established on the next send. + _transport.InvalidateCachedConnection(); } } } @@ -190,9 +190,9 @@ public int Port { _port = value; _servicePoint = null; - // The cached connection targets the previous port, so release it - // to force a new connection to be established on the next send. - _transport.ReleaseConnection(); + // The cached connection targets the previous port, so invalidate it to force + // a new connection to be established on the next send. + _transport.InvalidateCachedConnection(); } } } @@ -340,7 +340,17 @@ public X509CertificateCollection ClientCertificates public string? TargetName { get { return _targetName; } - set { _targetName = value; } + set + { + if (value != _targetName) + { + _targetName = value; + // The target name participates in connection establishment (authentication + // and TLS), so invalidate any cached connection to force a new one on the + // next send. + _transport.InvalidateCachedConnection(); + } + } } private bool ServerSupportsEai diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs index 6d678d0f5800ac..a100f4bd1fa67c 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs @@ -20,6 +20,7 @@ internal sealed class SmtpTransport private readonly SmtpClient _client; private ICredentialsByHost? _credentials; private bool _shouldAbort; + private bool _stale; private bool _enableSsl; @@ -43,7 +44,11 @@ internal ICredentialsByHost? Credentials } set { - _credentials = value; + if (!ReferenceEquals(value, _credentials)) + { + _credentials = value; + InvalidateCachedConnection(); + } } } @@ -51,7 +56,7 @@ internal bool IsConnected { get { - return _connection != null && _connection.IsConnected; + return _connection != null && _connection.IsConnected && !_stale; } } @@ -63,7 +68,11 @@ internal bool EnableSsl } set { - _enableSsl = value; + if (value != _enableSsl) + { + _enableSsl = value; + InvalidateCachedConnection(); + } } } @@ -89,6 +98,12 @@ internal Task GetConnectionAsync(string host, int port, Cancellation { lock (this) { + // Gracefully release any previously cached connection (for example one that + // became stale after a configuration change) before establishing a new one so + // its socket is not leaked. This keeps the potentially blocking shutdown work on + // the send path rather than in the property setters that invalidated it. + _connection?.ReleaseConnection(); + _stale = false; _connection = new SmtpConnection(this, _client, _credentials, _authenticationModules); if (_shouldAbort) { @@ -147,6 +162,15 @@ internal void ReleaseConnection() _connection?.ReleaseConnection(); } + // Marks any cached connection as stale without performing blocking work. The connection + // is gracefully released and replaced the next time one is established (see + // GetConnectionAsync). This is called when a property that affects how the connection is + // established (host, port, credentials, SSL settings, target name) changes. + internal void InvalidateCachedConnection() + { + _stale = true; + } + internal void Abort() { lock (this) diff --git a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs index 86d89c50fdd743..0519282942ec94 100644 --- a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs +++ b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs @@ -9,6 +9,13 @@ namespace System.Net.Mail.Tests { + public enum ConnectionAffectingProperty + { + Host, + Credentials, + TargetName, + } + public abstract class SmtpClientConnectionTest : LoopbackServerTestBase where TSendMethod : ISendMethodProvider { @@ -106,8 +113,11 @@ public async Task ChangingPort_DoesNotReuseConnectionToPreviousServer() Assert.Equal("", Server.MailFrom); } - [Fact] - public async Task ChangingHost_EstablishesNewConnection() + [Theory] + [InlineData(ConnectionAffectingProperty.Host)] + [InlineData(ConnectionAffectingProperty.Credentials)] + [InlineData(ConnectionAffectingProperty.TargetName)] + public async Task ChangingConnectionProperty_EstablishesNewConnection(ConnectionAffectingProperty property) { Server.ReceiveMultipleConnections = true; @@ -115,9 +125,21 @@ public async Task ChangingHost_EstablishesNewConnection() Assert.Equal(1, Server.ConnectionCount); Assert.Equal("", Server.MailFrom); - // Change the host to another value that still resolves to the loopback server. - // The cached connection must be dropped and a new one established. - Smtp.Host = "127.0.0.1"; + // Change a property that affects how the connection is established. The cached + // connection must be invalidated and a new one established on the next send. + switch (property) + { + case ConnectionAffectingProperty.Host: + // A different value that still resolves to the loopback server. + Smtp.Host = "127.0.0.1"; + break; + case ConnectionAffectingProperty.Credentials: + Smtp.Credentials = new NetworkCredential("user", "password"); + break; + case ConnectionAffectingProperty.TargetName: + Smtp.TargetName = "SMTPSVC/example.com"; + break; + } await SendMail(new MailMessage("second@example.com", "everyone@novell.com", "introduction", "hello")); Assert.Equal(2, Server.ConnectionCount); diff --git a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTlsTest.cs b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTlsTest.cs index 0ca6393f1f6cab..262a01e06f7919 100644 --- a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTlsTest.cs +++ b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTlsTest.cs @@ -185,6 +185,27 @@ public async Task ClientCertificateRequired_Sent() Assert.Equal(clientCert, receivedClientCert); } + [ActiveIssue("https://github.com/dotnet/runtime/issues/120959", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot), nameof(PlatformDetection.IsAndroid))] + [Fact] + public async Task EnableSsl_ChangedAfterConnect_EstablishesNewEncryptedConnection() + { + Server.ReceiveMultipleConnections = true; + _serverCertValidationCallback = (cert, chain, errors) => true; + + // First send happens over a plaintext connection. + await SendMail(new MailMessage("foo@example.com", "bar@example.com", "hello", "howdydoo")); + Assert.Equal(1, Server.ConnectionCount); + Assert.False(Server.IsEncrypted, "First connection should not be encrypted."); + + // Enabling SSL must invalidate the cached plaintext connection so the next send + // establishes a new, encrypted connection instead of reusing the old one. + Smtp.EnableSsl = true; + + await SendMail(new MailMessage("foo@example.com", "bar@example.com", "hello", "howdydoo")); + Assert.Equal(2, Server.ConnectionCount); + Assert.True(Server.IsEncrypted, "Second connection should be encrypted after enabling SSL."); + } + private bool ServerCertValidationCallback(object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors) { if (_serverCertValidationCallback != null) From e2d09c350c68ab12a0e125313bedd4c3227d3856 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 26 Aug 2026 12:04:35 +0200 Subject: [PATCH 03/10] Keep default TargetName in sync when Host changes Addresses review feedback: the default TargetName is derived once from the host ("SMTPSVC/"). When Host changed, the cached connection was invalidated but TargetName kept targeting the previous host, causing a Negotiate/NTLM SPN mismatch on the new connection. Now, when Host changes and TargetName still holds the host-derived default, TargetName is updated to match the new host. A TargetName explicitly set by the caller is left untouched. Add test coverage for both the default-follows- host and explicit-preservation cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d288b6ab-b489-4bb8-afb1-5552f3bae55d --- .../src/System/Net/Mail/SmtpClient.cs | 12 +++++++++++- .../Functional/SmtpClientConnectionTest.cs | 19 ++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs index b73959054262a5..e2008a708c7dc6 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs @@ -43,6 +43,7 @@ public class SmtpClient : IDisposable private bool _inCall; private bool _timedOut; private string? _targetName; + private const string DefaultTargetNamePrefix = "SMTPSVC/"; private SmtpDeliveryMethod _deliveryMethod = SmtpDeliveryMethod.Network; private SmtpDeliveryFormat _deliveryFormat = SmtpDeliveryFormat.SevenBit; // Non-EAI default private string? _pickupDirectoryLocation; @@ -103,7 +104,7 @@ private void Initialize() _port = DefaultPort; } - _targetName ??= "SMTPSVC/" + _host; + _targetName ??= DefaultTargetNamePrefix + _host; if (_clientDomain == null) { @@ -162,6 +163,15 @@ public string? Host if (value != _host) { + // If TargetName is still the default derived from the current host, keep it in + // sync with the new host so Negotiate/NTLM authentication uses the correct SPN. + // A TargetName explicitly set by the caller (not matching the default) is left + // untouched. + if (_targetName == DefaultTargetNamePrefix + _host) + { + _targetName = DefaultTargetNamePrefix + value; + } + _host = value; _servicePoint = null; // The cached connection targets the previous host, so invalidate it to force diff --git a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs index 0519282942ec94..c0d91855e66d2d 100644 --- a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs +++ b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs @@ -130,8 +130,11 @@ public async Task ChangingConnectionProperty_EstablishesNewConnection(Connection switch (property) { case ConnectionAffectingProperty.Host: - // A different value that still resolves to the loopback server. + // A different value that still resolves to the loopback server. The default + // TargetName should follow the host so authentication uses the correct SPN. + Assert.Equal("SMTPSVC/localhost", Smtp.TargetName); Smtp.Host = "127.0.0.1"; + Assert.Equal("SMTPSVC/127.0.0.1", Smtp.TargetName); break; case ConnectionAffectingProperty.Credentials: Smtp.Credentials = new NetworkCredential("user", "password"); @@ -145,6 +148,20 @@ public async Task ChangingConnectionProperty_EstablishesNewConnection(Connection Assert.Equal(2, Server.ConnectionCount); Assert.Equal("", Server.MailFrom); } + + [Fact] + public async Task ChangingHost_PreservesExplicitlySetTargetName() + { + // A TargetName explicitly set by the caller must not be overwritten when the host + // changes, even though the default (host-derived) TargetName does follow the host. + Smtp.TargetName = "SMTPSVC/explicit.example.com"; + + await SendMail(new MailMessage("first@example.com", "everyone@novell.com", "introduction", "hello")); + Assert.Equal("SMTPSVC/explicit.example.com", Smtp.TargetName); + + Smtp.Host = "127.0.0.1"; + Assert.Equal("SMTPSVC/explicit.example.com", Smtp.TargetName); + } } public class SmtpClientConnectionTest_Send : SmtpClientConnectionTest From 9eea71fadb47606c2084500e7505a8c721b71c54 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 26 Aug 2026 12:19:59 +0200 Subject: [PATCH 04/10] Abort failed cached connections and align test credential literal When GetConnectionAsync releases a previously cached SmtpConnection, only gracefully QUIT one that was actually established; abort (force-close) a connection whose connect attempt failed, since it may have no initialized stream and the graceful path would dereference it and mask the original failure. Also switch the connection test credential literal to the "foo"/"bar" convention used throughout the mail tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d288b6ab-b489-4bb8-afb1-5552f3bae55d --- .../src/System/Net/Mail/SmtpTransport.cs | 23 +++++++++++++++---- .../Functional/SmtpClientConnectionTest.cs | 2 +- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs index a100f4bd1fa67c..6686c5c22a5622 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs @@ -98,11 +98,24 @@ internal Task GetConnectionAsync(string host, int port, Cancellation { lock (this) { - // Gracefully release any previously cached connection (for example one that - // became stale after a configuration change) before establishing a new one so - // its socket is not leaked. This keeps the potentially blocking shutdown work on - // the send path rather than in the property setters that invalidated it. - _connection?.ReleaseConnection(); + // Release any previously cached connection (for example one that became stale + // after a configuration change, or one whose connect attempt failed) before + // establishing a new one so its socket is not leaked. Only a connection that was + // actually established can be shut down gracefully (QUIT); a connection that never + // connected may not have an initialized stream, so it is aborted instead. This + // keeps the potentially blocking shutdown work on the send path rather than in the + // property setters that invalidated it. + if (_connection is not null) + { + if (_connection.IsConnected) + { + _connection.ReleaseConnection(); + } + else + { + _connection.Abort(); + } + } _stale = false; _connection = new SmtpConnection(this, _client, _credentials, _authenticationModules); if (_shouldAbort) diff --git a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs index c0d91855e66d2d..743cef1574e3ef 100644 --- a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs +++ b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs @@ -137,7 +137,7 @@ public async Task ChangingConnectionProperty_EstablishesNewConnection(Connection Assert.Equal("SMTPSVC/127.0.0.1", Smtp.TargetName); break; case ConnectionAffectingProperty.Credentials: - Smtp.Credentials = new NetworkCredential("user", "password"); + Smtp.Credentials = new NetworkCredential("foo", "bar"); break; case ConnectionAffectingProperty.TargetName: Smtp.TargetName = "SMTPSVC/example.com"; From 198732dd43cfe1801633d14c420790f438822129 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 26 Aug 2026 12:33:14 +0200 Subject: [PATCH 05/10] Throw when TargetName is set during an in-progress send Now that TargetName is connection-affecting and invalidates the cached connection, guard its setter with the same _inCall check used by Host, Port, Credentials, and Timeout so it throws SmtpInvalidOperationDuringSend rather than mutating connection settings mid-send. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d288b6ab-b489-4bb8-afb1-5552f3bae55d --- .../System.Net.Mail/src/System/Net/Mail/SmtpClient.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs index e2008a708c7dc6..c0fa6f2e71fa05 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs @@ -352,6 +352,11 @@ public string? TargetName get { return _targetName; } set { + if (_inCall) + { + throw new InvalidOperationException(SR.SmtpInvalidOperationDuringSend); + } + if (value != _targetName) { _targetName = value; From 513757035d11be4c65220e4390cd2da293d38ce4 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 26 Aug 2026 12:38:43 +0200 Subject: [PATCH 06/10] Make SmtpTransport._stale volatile for cross-thread visibility The stale flag is written from property setters without holding the transport lock and read from IsConnected on the send path. Marking it volatile makes an invalidating configuration change reliably observable across threads without widening locking, so a stale connection is not reused after invalidation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d288b6ab-b489-4bb8-afb1-5552f3bae55d --- .../System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs index 6686c5c22a5622..69b2a6f8f35721 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs @@ -20,7 +20,10 @@ internal sealed class SmtpTransport private readonly SmtpClient _client; private ICredentialsByHost? _credentials; private bool _shouldAbort; - private bool _stale; + // Written (set true) from property setters without holding the transport lock and read + // from IsConnected on the send path, so it is volatile to make an invalidating + // configuration change reliably observable across threads without widening locking. + private volatile bool _stale; private bool _enableSsl; From 10baf0ce6f98aa4c797f6ef9a55ea0c937916f41 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 26 Aug 2026 12:57:57 +0200 Subject: [PATCH 07/10] Throw when EnableSsl is set during an in-progress send EnableSsl is connection-affecting and invalidates the cached connection, so guard its setter with the same _inCall check used by Host, Port, Credentials, Timeout, and TargetName to prevent toggling SSL mid-send. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d288b6ab-b489-4bb8-afb1-5552f3bae55d --- .../System.Net.Mail/src/System/Net/Mail/SmtpClient.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs index c0fa6f2e71fa05..d6d37432219c35 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs @@ -332,6 +332,11 @@ public bool EnableSsl } set { + if (_inCall) + { + throw new InvalidOperationException(SR.SmtpInvalidOperationDuringSend); + } + _transport.EnableSsl = value; } } From 840f468a8804ebcb1663bb90984f4f2efcc10d97 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 26 Aug 2026 13:04:09 +0200 Subject: [PATCH 08/10] Release stale connection outside the transport lock GetConnectionAsync gracefully releasing a previously cached connection performs a blocking QUIT over the network. Holding the transport lock across that I/O delayed a concurrent Abort() (for example from the send-timeout path). Detach the previous connection under the lock, shut it down outside the lock, then reacquire the lock to create the new connection so an Abort() during shutdown still flows through _shouldAbort. Sends are serialized by SmtpClient._inCall, so no other GetConnectionAsync runs concurrently, and ShutdownConnection is idempotent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d288b6ab-b489-4bb8-afb1-5552f3bae55d --- .../src/System/Net/Mail/SmtpTransport.cs | 44 ++++++++++++------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs index 69b2a6f8f35721..4027dd4c971180 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs @@ -99,27 +99,37 @@ internal async Task GetConnectionAsync(string host, int port, CancellationToken internal Task GetConnectionAsync(string host, int port, CancellationToken cancellationToken = default) where TIOAdapter : IReadWriteAdapter { + // Detach any previously cached connection (for example one that became stale after a + // configuration change, or one whose connect attempt failed) so its socket is not + // leaked. Sends are serialized by SmtpClient._inCall, so no other GetConnectionAsync + // can run concurrently here. + SmtpConnection? previousConnection; lock (this) { - // Release any previously cached connection (for example one that became stale - // after a configuration change, or one whose connect attempt failed) before - // establishing a new one so its socket is not leaked. Only a connection that was - // actually established can be shut down gracefully (QUIT); a connection that never - // connected may not have an initialized stream, so it is aborted instead. This - // keeps the potentially blocking shutdown work on the send path rather than in the - // property setters that invalidated it. - if (_connection is not null) + previousConnection = _connection; + _connection = null; + _stale = false; + } + + // Shut the previous connection down outside the lock: a graceful release performs a + // blocking QUIT over the network, and holding the transport lock across that I/O would + // delay a concurrent Abort() (for example from the send-timeout path). Only a + // connection that was actually established can be shut down gracefully; one that never + // connected may not have an initialized stream, so it is aborted instead. + if (previousConnection is not null) + { + if (previousConnection.IsConnected) { - if (_connection.IsConnected) - { - _connection.ReleaseConnection(); - } - else - { - _connection.Abort(); - } + previousConnection.ReleaseConnection(); } - _stale = false; + else + { + previousConnection.Abort(); + } + } + + lock (this) + { _connection = new SmtpConnection(this, _client, _credentials, _authenticationModules); if (_shouldAbort) { From 44c4b9b37bab443d4d7110d69843a1805db67772 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 26 Aug 2026 13:16:05 +0200 Subject: [PATCH 09/10] Clarify TargetName comment: SPN for authentication, not TLS Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d288b6ab-b489-4bb8-afb1-5552f3bae55d --- .../System.Net.Mail/src/System/Net/Mail/SmtpClient.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs index d6d37432219c35..372fd2b06ce932 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs @@ -365,9 +365,8 @@ public string? TargetName if (value != _targetName) { _targetName = value; - // The target name participates in connection establishment (authentication - // and TLS), so invalidate any cached connection to force a new one on the - // next send. + // The target name is the SPN used during authentication, so invalidate any + // cached connection to force a new one on the next send. _transport.InvalidateCachedConnection(); } } From 54a17eec808617112b4fa8a2a69834e272cddc8e Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 26 Aug 2026 13:34:31 +0200 Subject: [PATCH 10/10] Abort stale connection instead of graceful QUIT in GetConnectionAsync A graceful ReleaseConnection() performs a synchronous blocking QUIT over the network on the async send path before the first await, which can block the caller thread. A connection being discarded due to a configuration change does not need a polite QUIT, so abort it instead. Because Abort() is non-blocking, the previous detach-outside-lock structure is no longer needed and the logic collapses back into a single lock. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d288b6ab-b489-4bb8-afb1-5552f3bae55d --- .../src/System/Net/Mail/SmtpTransport.cs | 42 +++++-------------- 1 file changed, 11 insertions(+), 31 deletions(-) diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs index 4027dd4c971180..19d01f8af9f838 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs @@ -99,37 +99,17 @@ internal async Task GetConnectionAsync(string host, int port, CancellationToken internal Task GetConnectionAsync(string host, int port, CancellationToken cancellationToken = default) where TIOAdapter : IReadWriteAdapter { - // Detach any previously cached connection (for example one that became stale after a - // configuration change, or one whose connect attempt failed) so its socket is not - // leaked. Sends are serialized by SmtpClient._inCall, so no other GetConnectionAsync - // can run concurrently here. - SmtpConnection? previousConnection; lock (this) { - previousConnection = _connection; - _connection = null; + // Abort any previously cached connection (for example one that became stale after a + // configuration change, or one whose connect attempt failed) so its socket is not + // leaked. Abort() only force-closes the socket without any network round-trip, so it + // is safe to run under the lock and does not block this async send path. A graceful + // QUIT is unnecessary for a connection we are discarding. Sends are serialized by + // SmtpClient._inCall, so no other GetConnectionAsync can run concurrently here. + _connection?.Abort(); _stale = false; - } - // Shut the previous connection down outside the lock: a graceful release performs a - // blocking QUIT over the network, and holding the transport lock across that I/O would - // delay a concurrent Abort() (for example from the send-timeout path). Only a - // connection that was actually established can be shut down gracefully; one that never - // connected may not have an initialized stream, so it is aborted instead. - if (previousConnection is not null) - { - if (previousConnection.IsConnected) - { - previousConnection.ReleaseConnection(); - } - else - { - previousConnection.Abort(); - } - } - - lock (this) - { _connection = new SmtpConnection(this, _client, _credentials, _authenticationModules); if (_shouldAbort) { @@ -188,10 +168,10 @@ internal void ReleaseConnection() _connection?.ReleaseConnection(); } - // Marks any cached connection as stale without performing blocking work. The connection - // is gracefully released and replaced the next time one is established (see - // GetConnectionAsync). This is called when a property that affects how the connection is - // established (host, port, credentials, SSL settings, target name) changes. + // Marks any cached connection as stale without performing blocking work. The connection is + // aborted and replaced the next time one is established (see GetConnectionAsync). This is + // called when a property that affects how the connection is established (host, port, + // credentials, SSL settings, target name) changes. internal void InvalidateCachedConnection() { _stale = true;