Skip to content

Commit

Permalink
Revert SMTP pipelining of the DATA command
Browse files Browse the repository at this point in the history
Fixes issue #1459
  • Loading branch information
jstedfast committed Nov 12, 2022
1 parent 3ceb8df commit da39694
Show file tree
Hide file tree
Showing 7 changed files with 22 additions and 171 deletions.
19 changes: 6 additions & 13 deletions MailKit/Net/Smtp/AsyncSmtpClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1005,10 +1005,10 @@ async Task<string> MessageDataAsync (FormatOptions options, MimeMessage message,
return ParseMessageDataResponse (message, response);
}

async Task ResetAsync (bool dataAccepted, CancellationToken cancellationToken)
async Task ResetAsync (CancellationToken cancellationToken)
{
try {
var response = await Stream.SendCommandAsync (dataAccepted ? ".\r\n" : "RSET\r\n", cancellationToken).ConfigureAwait (false);
var response = await Stream.SendCommandAsync ("RSET\r\n", cancellationToken).ConfigureAwait (false);
if (response.StatusCode != SmtpStatusCode.Ok)
Disconnect (uri.Host, uri.Port, GetSecureSocketOptions (uri), false);
} catch (SmtpCommandException) {
Expand Down Expand Up @@ -1043,17 +1043,13 @@ async Task<string> SendAsync (FormatOptions options, MimeMessage message, Mailbo
recipientsAccepted++;
}

if (!bdat && pipeline)
await QueueCommandAsync (SmtpCommand.Data, "DATA\r\n", cancellationToken).ConfigureAwait (false);

if (queued.Count > 0) {
// Note: if PIPELINING is supported, this will flush all outstanding
// MAIL FROM, RCPT TO, and DATA commands to the server and then process
// MAIL FROM and RCPT TO commands to the server and then process
// all of their responses.
var results = await FlushCommandQueueAsync (message, sender, recipients, cancellationToken).ConfigureAwait (false);

recipientsAccepted = results.RecipientsAccepted;
dataResponse = results.DataResponse;

if (results.FirstException != null)
throw results.FirstException;
Expand All @@ -1067,21 +1063,18 @@ async Task<string> SendAsync (FormatOptions options, MimeMessage message, Mailbo
if (bdat)
return await BdatAsync (format, message, size, cancellationToken, progress).ConfigureAwait (false);

if (!pipeline)
dataResponse = await Stream.SendCommandAsync ("DATA\r\n", cancellationToken).ConfigureAwait (false);
dataResponse = await Stream.SendCommandAsync ("DATA\r\n", cancellationToken).ConfigureAwait (false);

ParseDataResponse (dataResponse);
dataResponse = null;

return await MessageDataAsync (format, message, size, cancellationToken, progress).ConfigureAwait (false);
} catch (ServiceNotAuthenticatedException) {
// do not disconnect
var dataAccepted = dataResponse != null && dataResponse.StatusCode == SmtpStatusCode.StartMailInput;
await ResetAsync (dataAccepted, cancellationToken).ConfigureAwait (false);
await ResetAsync (cancellationToken).ConfigureAwait (false);
throw;
} catch (SmtpCommandException) {
var dataAccepted = dataResponse != null && dataResponse.StatusCode == SmtpStatusCode.StartMailInput;
await ResetAsync (dataAccepted, cancellationToken).ConfigureAwait (false);
await ResetAsync (cancellationToken).ConfigureAwait (false);
throw;
} catch {
Disconnect (uri.Host, uri.Port, GetSecureSocketOptions (uri), false);
Expand Down
34 changes: 10 additions & 24 deletions MailKit/Net/Smtp/SmtpClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ public partial class SmtpClient : MailTransport, ISmtpClient

enum SmtpCommand {
MailFrom,
RcptTo,
Data
RcptTo
}

readonly HashSet<string> authenticationMechanisms = new HashSet<string> (StringComparer.Ordinal);
Expand Down Expand Up @@ -580,20 +579,17 @@ void QueueCommand (SmtpCommand type, string command, CancellationToken cancellat
struct QueueResults
{
public readonly int RecipientsAccepted;
public readonly SmtpResponse DataResponse;
public Exception FirstException;

public QueueResults (int recipientsAccepted, SmtpResponse dataResponse, Exception firstException)
public QueueResults (int recipientsAccepted, Exception firstException)
{
RecipientsAccepted = recipientsAccepted;
DataResponse = dataResponse;
FirstException = firstException;
}
}

QueueResults ParseCommandQueueResponses (MimeMessage message, MailboxAddress sender, IList<MailboxAddress> recipients, List<SmtpResponse> responses, Exception readResponseException)
{
SmtpResponse dataResponse = null;
Exception firstException = null;
int recipientsAccepted = 0;
int rcpt = 0;
Expand All @@ -617,16 +613,13 @@ QueueResults ParseCommandQueueResponses (MimeMessage message, MailboxAddress sen
firstException ??= ex;
}
break;
case SmtpCommand.Data:
dataResponse = responses[i];
break;
}
}
} finally {
queued.Clear ();
}

return new QueueResults (recipientsAccepted, dataResponse, firstException ?? readResponseException);
return new QueueResults (recipientsAccepted, firstException ?? readResponseException);
}

QueueResults FlushCommandQueue (MimeMessage message, MailboxAddress sender, IList<MailboxAddress> recipients, CancellationToken cancellationToken)
Expand Down Expand Up @@ -2282,10 +2275,10 @@ string MessageData (FormatOptions options, MimeMessage message, long size, Cance
return ParseMessageDataResponse (message, response);
}

void Reset (bool dataAccepted, CancellationToken cancellationToken)
void Reset (CancellationToken cancellationToken)
{
try {
var response = Stream.SendCommand (dataAccepted ? ".\r\n" : "RSET\r\n", cancellationToken);
var response = Stream.SendCommand ("RSET\r\n", cancellationToken);
if (response.StatusCode != SmtpStatusCode.Ok)
Disconnect (uri.Host, uri.Port, GetSecureSocketOptions (uri), false);
} catch (SmtpCommandException) {
Expand Down Expand Up @@ -2418,17 +2411,13 @@ string Send (FormatOptions options, MimeMessage message, MailboxAddress sender,
recipientsAccepted++;
}

if (!bdat && pipeline)
QueueCommand (SmtpCommand.Data, "DATA\r\n", cancellationToken);

if (queued.Count > 0) {
// Note: if PIPELINING is supported, this will flush all outstanding
// MAIL FROM, RCPT TO, and DATA commands to the server and then process
// MAIL FROM and RCPT TO commands to the server and then process
// all of their responses.
var results = FlushCommandQueue (message, sender, recipients, cancellationToken);

recipientsAccepted = results.RecipientsAccepted;
dataResponse = results.DataResponse;

if (results.FirstException != null)
throw results.FirstException;
Expand All @@ -2442,21 +2431,18 @@ string Send (FormatOptions options, MimeMessage message, MailboxAddress sender,
if (bdat)
return Bdat (format, message, size, cancellationToken, progress);

if (!pipeline)
dataResponse = Stream.SendCommand ("DATA\r\n", cancellationToken);
dataResponse = Stream.SendCommand ("DATA\r\n", cancellationToken);

ParseDataResponse (dataResponse);
dataResponse = null;

return MessageData (format, message, size, cancellationToken, progress);
} catch (ServiceNotAuthenticatedException) {
// do not disconnect
var dataAccepted = dataResponse != null && dataResponse.StatusCode == SmtpStatusCode.StartMailInput;
Reset (dataAccepted, cancellationToken);
Reset (cancellationToken);
throw;
} catch (SmtpCommandException) {
var dataAccepted = dataResponse != null && dataResponse.StatusCode == SmtpStatusCode.StartMailInput;
Reset (dataAccepted, cancellationToken);
Reset (cancellationToken);
throw;
} catch {
Disconnect (uri.Host, uri.Port, GetSecureSocketOptions (uri), false);
Expand Down Expand Up @@ -2626,7 +2612,7 @@ List<MailboxAddress> ValidateArguments (FormatOptions options, MimeMessage messa
return Send (options, message, sender, rcpts, cancellationToken, progress);
}

#endregion
#endregion

string CreateExpandCommand (string alias)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
250 2.1.0 sender ok
250 2.1.5 recipient ok
354 enter mail, end with . on a line by itself

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
250 2.1.0 sender ok
550 5.1.0 mailbox unavailable
554 no valid recipients given
134 changes: 6 additions & 128 deletions UnitTests/Net/Smtp/SmtpClientTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2671,7 +2671,8 @@ public void TestPipelining (bool showProgress)
commands.Add (new SmtpReplayCommand ("", "comcast-greeting.txt"));
commands.Add (new SmtpReplayCommand ($"EHLO {SmtpClient.DefaultLocalDomain}\r\n", "comcast-ehlo+pipelining.txt"));
commands.Add (new SmtpReplayCommand ("AUTH PLAIN AHVzZXJuYW1lAHBhc3N3b3Jk\r\n", "comcast-auth-plain.txt"));
commands.Add (new SmtpReplayCommand ("MAIL FROM:<[email protected]> BODY=8BITMIME\r\nRCPT TO:<[email protected]>\r\nDATA\r\n", "pipelined-mail-from-rcpt-to.txt"));
commands.Add (new SmtpReplayCommand ("MAIL FROM:<[email protected]> BODY=8BITMIME\r\nRCPT TO:<[email protected]>\r\n", "pipelined-mail-from-rcpt-to.txt"));
commands.Add (new SmtpReplayCommand ("DATA\r\n", "comcast-data.txt"));
commands.Add (new SmtpReplayCommand (".\r\n", "comcast-data-done.txt"));
commands.Add (new SmtpReplayCommand ("QUIT\r\n", "comcast-quit.txt"));

Expand Down Expand Up @@ -2735,7 +2736,8 @@ public async Task TestPipeliningAsync (bool showProgress)
commands.Add (new SmtpReplayCommand ("", "comcast-greeting.txt"));
commands.Add (new SmtpReplayCommand ($"EHLO {SmtpClient.DefaultLocalDomain}\r\n", "comcast-ehlo+pipelining.txt"));
commands.Add (new SmtpReplayCommand ("AUTH PLAIN AHVzZXJuYW1lAHBhc3N3b3Jk\r\n", "comcast-auth-plain.txt"));
commands.Add (new SmtpReplayCommand ("MAIL FROM:<[email protected]> BODY=8BITMIME\r\nRCPT TO:<[email protected]>\r\nDATA\r\n", "pipelined-mail-from-rcpt-to.txt"));
commands.Add (new SmtpReplayCommand ("MAIL FROM:<[email protected]> BODY=8BITMIME\r\nRCPT TO:<[email protected]>\r\n", "pipelined-mail-from-rcpt-to.txt"));
commands.Add (new SmtpReplayCommand ("DATA\r\n", "comcast-data.txt"));
commands.Add (new SmtpReplayCommand (".\r\n", "comcast-data-done.txt"));
commands.Add (new SmtpReplayCommand ("QUIT\r\n", "comcast-quit.txt"));

Expand Down Expand Up @@ -3175,7 +3177,7 @@ public void TestNoRecipientsAcceptedPipelined ()
commands.Add (new SmtpReplayCommand ("", "comcast-greeting.txt"));
commands.Add (new SmtpReplayCommand ($"EHLO {SmtpClient.DefaultLocalDomain}\r\n", "comcast-ehlo+pipelining.txt"));
commands.Add (new SmtpReplayCommand ("AUTH PLAIN AHVzZXJuYW1lAHBhc3N3b3Jk\r\n", "comcast-auth-plain.txt"));
commands.Add (new SmtpReplayCommand ("MAIL FROM:<[email protected]>\r\nRCPT TO:<[email protected]>\r\nDATA\r\n", "pipelined-mailbox-unavailable.txt", SmtpReplayState.WaitForCommand));
commands.Add (new SmtpReplayCommand ("MAIL FROM:<[email protected]>\r\nRCPT TO:<[email protected]>\r\n", "pipelined-mailbox-unavailable.txt"));
commands.Add (new SmtpReplayCommand ("RSET\r\n", "comcast-rset.txt"));
commands.Add (new SmtpReplayCommand ("QUIT\r\n", "comcast-quit.txt"));

Expand Down Expand Up @@ -3237,7 +3239,7 @@ public async Task TestNoRecipientsAcceptedPipelinedAsync ()
commands.Add (new SmtpReplayCommand ("", "comcast-greeting.txt"));
commands.Add (new SmtpReplayCommand ($"EHLO {SmtpClient.DefaultLocalDomain}\r\n", "comcast-ehlo+pipelining.txt"));
commands.Add (new SmtpReplayCommand ("AUTH PLAIN AHVzZXJuYW1lAHBhc3N3b3Jk\r\n", "comcast-auth-plain.txt"));
commands.Add (new SmtpReplayCommand ("MAIL FROM:<[email protected]>\r\nRCPT TO:<[email protected]>\r\nDATA\r\n", "pipelined-mailbox-unavailable.txt", SmtpReplayState.WaitForCommand));
commands.Add (new SmtpReplayCommand ("MAIL FROM:<[email protected]>\r\nRCPT TO:<[email protected]>\r\n", "pipelined-mailbox-unavailable.txt"));
commands.Add (new SmtpReplayCommand ("RSET\r\n", "comcast-rset.txt"));
commands.Add (new SmtpReplayCommand ("QUIT\r\n", "comcast-quit.txt"));

Expand Down Expand Up @@ -3292,130 +3294,6 @@ public async Task TestNoRecipientsAcceptedPipelinedAsync ()
}
}

[Test]
public void TestNoRecipientsAcceptedButDataAcceptedPipelined ()
{
var commands = new List<SmtpReplayCommand> ();
commands.Add (new SmtpReplayCommand ("", "comcast-greeting.txt"));
commands.Add (new SmtpReplayCommand ($"EHLO {SmtpClient.DefaultLocalDomain}\r\n", "comcast-ehlo+pipelining.txt"));
commands.Add (new SmtpReplayCommand ("AUTH PLAIN AHVzZXJuYW1lAHBhc3N3b3Jk\r\n", "comcast-auth-plain.txt"));
commands.Add (new SmtpReplayCommand ("MAIL FROM:<[email protected]>\r\nRCPT TO:<[email protected]>\r\nDATA\r\n", "pipelined-mailbox-unavailable-data-accepted.txt"));
commands.Add (new SmtpReplayCommand (".\r\n", "comcast-data-done.txt"));
commands.Add (new SmtpReplayCommand ("QUIT\r\n", "comcast-quit.txt"));

using (var client = new NoRecipientsAcceptedSmtpClient ()) {
try {
client.ReplayConnect ("localhost", new SmtpReplayStream (commands, false));
} catch (Exception ex) {
Assert.Fail ("Did not expect an exception in Connect: {0}", ex);
}

Assert.IsTrue (client.IsConnected, "Client failed to connect.");

Assert.IsTrue (client.Capabilities.HasFlag (SmtpCapabilities.Authentication), "Failed to detect AUTH extension");
Assert.IsTrue (client.AuthenticationMechanisms.Contains ("LOGIN"), "Failed to detect the LOGIN auth mechanism");
Assert.IsTrue (client.AuthenticationMechanisms.Contains ("PLAIN"), "Failed to detect the PLAIN auth mechanism");

Assert.IsTrue (client.Capabilities.HasFlag (SmtpCapabilities.EightBitMime), "Failed to detect 8BITMIME extension");
Assert.IsTrue (client.Capabilities.HasFlag (SmtpCapabilities.EnhancedStatusCodes), "Failed to detect ENHANCEDSTATUSCODES extension");
Assert.IsTrue (client.Capabilities.HasFlag (SmtpCapabilities.Size), "Failed to detect SIZE extension");
Assert.AreEqual (36700160, client.MaxSize, "Failed to parse SIZE correctly");
Assert.IsTrue (client.Capabilities.HasFlag (SmtpCapabilities.StartTLS), "Failed to detect STARTTLS extension");

try {
client.Authenticate ("username", "password");
} catch (Exception ex) {
Assert.Fail ("Did not expect an exception in Authenticate: {0}", ex);
}

try {
using (var message = CreateSimpleMessage ())
client.Send (message);
Assert.Fail ("Expected an SmtpException");
} catch (SmtpCommandException sex) {
Assert.AreEqual (SmtpErrorCode.MessageNotAccepted, sex.ErrorCode, "Unexpected SmtpErrorCode");
Assert.AreEqual (SmtpStatusCode.TransactionFailed, sex.StatusCode, "Unexpected SmtpStatusCode");
} catch (Exception ex) {
Assert.Fail ("Did not expect this exception in Send: {0}", ex);
}

Assert.AreEqual (1, client.NotAccepted, "NotAccepted");
Assert.IsTrue (client.NoRecipientsAccepted, "NoRecipientsAccepted");

Assert.IsTrue (client.IsConnected, "Expected the client to still be connected");

try {
client.Disconnect (true);
} catch (Exception ex) {
Assert.Fail ("Did not expect an exception in Disconnect: {0}", ex);
}

Assert.IsFalse (client.IsConnected, "Failed to disconnect");
}
}

[Test]
public async Task TestNoRecipientsAcceptedButDataAcceptedPipelinedAsync ()
{
var commands = new List<SmtpReplayCommand> ();
commands.Add (new SmtpReplayCommand ("", "comcast-greeting.txt"));
commands.Add (new SmtpReplayCommand ($"EHLO {SmtpClient.DefaultLocalDomain}\r\n", "comcast-ehlo+pipelining.txt"));
commands.Add (new SmtpReplayCommand ("AUTH PLAIN AHVzZXJuYW1lAHBhc3N3b3Jk\r\n", "comcast-auth-plain.txt"));
commands.Add (new SmtpReplayCommand ("MAIL FROM:<[email protected]>\r\nRCPT TO:<[email protected]>\r\nDATA\r\n", "pipelined-mailbox-unavailable-data-accepted.txt"));
commands.Add (new SmtpReplayCommand (".\r\n", "comcast-data-done.txt"));
commands.Add (new SmtpReplayCommand ("QUIT\r\n", "comcast-quit.txt"));

using (var client = new NoRecipientsAcceptedSmtpClient ()) {
try {
await client.ReplayConnectAsync ("localhost", new SmtpReplayStream (commands, true));
} catch (Exception ex) {
Assert.Fail ("Did not expect an exception in Connect: {0}", ex);
}

Assert.IsTrue (client.IsConnected, "Client failed to connect.");

Assert.IsTrue (client.Capabilities.HasFlag (SmtpCapabilities.Authentication), "Failed to detect AUTH extension");
Assert.IsTrue (client.AuthenticationMechanisms.Contains ("LOGIN"), "Failed to detect the LOGIN auth mechanism");
Assert.IsTrue (client.AuthenticationMechanisms.Contains ("PLAIN"), "Failed to detect the PLAIN auth mechanism");

Assert.IsTrue (client.Capabilities.HasFlag (SmtpCapabilities.EightBitMime), "Failed to detect 8BITMIME extension");
Assert.IsTrue (client.Capabilities.HasFlag (SmtpCapabilities.EnhancedStatusCodes), "Failed to detect ENHANCEDSTATUSCODES extension");
Assert.IsTrue (client.Capabilities.HasFlag (SmtpCapabilities.Size), "Failed to detect SIZE extension");
Assert.AreEqual (36700160, client.MaxSize, "Failed to parse SIZE correctly");
Assert.IsTrue (client.Capabilities.HasFlag (SmtpCapabilities.StartTLS), "Failed to detect STARTTLS extension");

try {
await client.AuthenticateAsync ("username", "password");
} catch (Exception ex) {
Assert.Fail ("Did not expect an exception in Authenticate: {0}", ex);
}

try {
using (var message = CreateSimpleMessage ())
await client.SendAsync (message);
Assert.Fail ("Expected an SmtpException");
} catch (SmtpCommandException sex) {
Assert.AreEqual (sex.ErrorCode, SmtpErrorCode.MessageNotAccepted, "Unexpected SmtpErrorCode");
Assert.AreEqual (sex.StatusCode, SmtpStatusCode.TransactionFailed, "Unexpected SmtpStatusCode");
} catch (Exception ex) {
Assert.Fail ("Did not expect this exception in Send: {0}", ex);
}

Assert.AreEqual (1, client.NotAccepted, "NotAccepted");
Assert.IsTrue (client.NoRecipientsAccepted, "NoRecipientsAccepted");

Assert.IsTrue (client.IsConnected, "Expected the client to still be connected");

try {
await client.DisconnectAsync (true);
} catch (Exception ex) {
Assert.Fail ("Did not expect an exception in Disconnect: {0}", ex);
}

Assert.IsFalse (client.IsConnected, "Failed to disconnect");
}
}

[Test]
public void TestUnauthorizedAccessException ()
{
Expand Down
1 change: 0 additions & 1 deletion UnitTests/UnitTests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,6 @@
<EmbeddedResource Include="Net\Smtp\Resources\greeting-not-ready.txt" />
<EmbeddedResource Include="Net\Smtp\Resources\helo.txt" />
<EmbeddedResource Include="Net\Smtp\Resources\mailbox-unavailable.txt" />
<EmbeddedResource Include="Net\Smtp\Resources\pipelined-mailbox-unavailable-data-accepted.txt" />
<EmbeddedResource Include="Net\Smtp\Resources\pipelined-mailbox-unavailable.txt" />
<EmbeddedResource Include="Net\Smtp\Resources\pipelined-mail-from-rcpt-to.txt" />
<EmbeddedResource Include="Net\Smtp\Resources\rfc0821-expn.txt" />
Expand Down

0 comments on commit da39694

Please sign in to comment.