(Relay) Handle unexpected replies and disconnections adequately
authorMegaBrutal <code+git@megabrutal.com>
Wed, 11 Feb 2015 23:04:50 +0000 (00:04 +0100)
committerMegaBrutal <code+git@megabrutal.com>
Wed, 11 Feb 2015 23:04:50 +0000 (00:04 +0100)
- Previously it was possible that MgSMTP erroneously administered
an e-mail as delivered, when the server accepted the recipient in
the RCPT stage, but disconnected before or in the DATA stage.
This patch mainly addresses this issue, by making sure that the
transaction is completed, before administering a final status.

- MgSMTP is now more tolerant with non-compliant replies,
e.g., accepts 250 for connection opening, not only 220.

- Increased relay send buffer size from 32 lines to 64.

modified:   Common.pas
modified:   MgSMTP.pas
modified:   Relay.pas
modified:   Spool.pas

Common.pas
MgSMTP.pas
Relay.pas
Spool.pas

index 8e0b7fb443ffa93db0871a7522e42a007bbc40f6..76926728bcbeddd3042f36a2bca0ff8be1e73b50 100644 (file)
@@ -138,6 +138,7 @@ type
       procedure AddRecipient(Recipient: TRecipient); overload;
       procedure SetReturnPath(Address: string);
       procedure SetRecipientData(Index, Data: integer; RMsg: string = '');
       procedure AddRecipient(Recipient: TRecipient); overload;
       procedure SetReturnPath(Address: string);
       procedure SetRecipientData(Index, Data: integer; RMsg: string = '');
+      procedure SetAllRecipientData(Data: integer; RMsg: string = '');
       procedure SetRelayHost(HostName: string);
       property ReturnPath: string read FReturnPath write SetReturnPath;
       property RelayHost: string read FRelayHost write SetRelayHost;
       procedure SetRelayHost(HostName: string);
       property ReturnPath: string read FReturnPath write SetReturnPath;
       property RelayHost: string read FRelayHost write SetRelayHost;
@@ -154,7 +155,7 @@ type
    function EMailTimeStamp(DateTime: TDateTime): string;
    function EMailTimeStampCorrected(DateTime: TDateTime): string;
    function StatusToStr(Status: integer): string;
    function EMailTimeStamp(DateTime: TDateTime): string;
    function EMailTimeStampCorrected(DateTime: TDateTime): string;
    function StatusToStr(Status: integer): string;
-   procedure AssignDeliveryStatusToSMTPCodes(Envelope: TEnvelope);
+   procedure AssignDeliveryStatusToSMTPCodes(Envelope: TEnvelope; TransactionComplete: boolean);
 
    function CleanEOLN(S: string): string;
    function GenerateRandomString(Length: integer): string;
 
    function CleanEOLN(S: string): string;
    function GenerateRandomString(Length: integer): string;
@@ -303,7 +304,7 @@ begin
       + '+' + IntToStr(Status and DS_SMTPREPLYMASK);
 end;
 
       + '+' + IntToStr(Status and DS_SMTPREPLYMASK);
 end;
 
-procedure AssignDeliveryStatusToSMTPCodes(Envelope: TEnvelope);
+procedure AssignDeliveryStatusToSMTPCodes(Envelope: TEnvelope; TransactionComplete: boolean);
 var i, code, cond, status: integer; Recipient: TRecipient;
 begin
    for i:= 0 to Envelope.GetNumberOfRecipients - 1 do begin
 var i, code, cond, status: integer; Recipient: TRecipient;
 begin
    for i:= 0 to Envelope.GetNumberOfRecipients - 1 do begin
@@ -312,7 +313,8 @@ begin
       cond:= code div 100;
       case cond of
          0: status:= DS_DELAYED or DS_UNEXPECTEDFAIL;
       cond:= code div 100;
       case cond of
          0: status:= DS_DELAYED or DS_UNEXPECTEDFAIL;
-         2: status:= DS_DELIVERED;
+         2: if TransactionComplete then status:= DS_DELIVERED
+            else status:= DS_DELAYED or DS_UNEXPECTEDFAIL;
          4: status:= DS_DELAYED;
          5: status:= DS_PERMANENT;
          else status:= DS_PERMANENT or DS_UNEXPECTEDFAIL;
          4: status:= DS_DELAYED;
          5: status:= DS_PERMANENT;
          else status:= DS_PERMANENT or DS_UNEXPECTEDFAIL;
@@ -658,6 +660,13 @@ begin
    FRecipients[Index].Data:= Data;
 end;
 
    FRecipients[Index].Data:= Data;
 end;
 
+procedure TEnvelope.SetAllRecipientData(Data: integer; RMsg: string = '');
+var i: integer;
+begin
+   for i:= 0 to Length(FRecipients) - 1 do
+      SetRecipientData(i, Data, RMsg);
+end;
+
 procedure TEnvelope.SetReturnPath(Address: string);
 begin
    FReturnPath:= Address;
 procedure TEnvelope.SetReturnPath(Address: string);
 begin
    FReturnPath:= Address;
index 181d969171d98cc44c1173ade478ec05d23ff81e..b172a1017abcce5a9bbf3cefcd84a40218a87d29 100644 (file)
@@ -49,7 +49,7 @@ const
      document what bugfix/feature are you testing with the actual build.
      This will be logged to help you differentiate outputs of subsequent
      builds in your logs. If left empty, it won't be added to the logs. }
      document what bugfix/feature are you testing with the actual build.
      This will be logged to help you differentiate outputs of subsequent
      builds in your logs. If left empty, it won't be added to the logs. }
-   DEVCOMMENT  =  'EHLO->HELO fallback';
+   DEVCOMMENT  =  'Detect unexpected disconnection';
 
 var
 
 
 var
 
index cea792d52c5237a026db4f17fb390c413e16fd1f..713db8d8641af8593d6703d4a9999bd6c548efd1 100644 (file)
--- a/Relay.pas
+++ b/Relay.pas
@@ -1,6 +1,6 @@
 {
    MegaBrutal's SMTP Server (MgSMTP)
 {
    MegaBrutal's SMTP Server (MgSMTP)
-   Copyright (C) 2010 MegaBrutal
+   Copyright (C) 2010-2015 MegaBrutal
 
    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU Affero General Public License as published by
 
    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU Affero General Public License as published by
@@ -88,6 +88,7 @@ type
    protected
       FEnvelope: TEnvelope;
       FEMailProperties: TEMailProperties;
    protected
       FEnvelope: TEnvelope;
       FEMailProperties: TEMailProperties;
+      FTransactionComplete: boolean;
       FRoutingTarget: TRoutingTarget;
       RoutingTable: TRoutingTable;
       TCP: TTCPRFCConnection;
       FRoutingTarget: TRoutingTarget;
       RoutingTable: TRoutingTable;
       TCP: TTCPRFCConnection;
@@ -99,6 +100,7 @@ type
    public
       property Envelope: TEnvelope read FEnvelope;
       property EMailProperties: TEMailProperties read FEMailProperties;
    public
       property Envelope: TEnvelope read FEnvelope;
       property EMailProperties: TEMailProperties read FEMailProperties;
+      property IsTransactionComplete: boolean read FTransactionComplete;
       property RelayServerName: string read GetRelayServerName;
       property RelayServerPort: integer read GetRelayServerPort;
       function OpenConnection: boolean;
       property RelayServerName: string read GetRelayServerName;
       property RelayServerPort: integer read GetRelayServerPort;
       function OpenConnection: boolean;
@@ -171,6 +173,7 @@ begin
    Self.RoutingTable:= RoutingTable;
    FEnvelope:= Envelope;
    FEMailProperties:= EMailProperties;
    Self.RoutingTable:= RoutingTable;
    FEnvelope:= Envelope;
    FEMailProperties:= EMailProperties;
+   FTransactionComplete:= false;
    FRoutingTarget:= RoutingTable.GetRouteInfo(Envelope.RelayHost);
    Response:= TRFCReply.Create;
    FillChar(SMTPExtensions, SizeOf(TSMTPExtensions), #0);
    FRoutingTarget:= RoutingTable.GetRouteInfo(Envelope.RelayHost);
    Response:= TRFCReply.Create;
    FillChar(SMTPExtensions, SizeOf(TSMTPExtensions), #0);
@@ -302,10 +305,8 @@ end;
 
 
 procedure TRelayer.AdministerMassFailure(var Result: boolean);
 
 
 procedure TRelayer.AdministerMassFailure(var Result: boolean);
-var i: integer;
 begin
 begin
-   for i:= 0 to Envelope.GetNumberOfRecipients - 1 do
-      Envelope.SetRecipientData(i, Response.GetNumericCode, Response.ReplyText.Text);
+   Envelope.SetAllRecipientData(Response.GetNumericCode, Response.ReplyText.Text);
    Result:= false;
 end;
 
    Result:= false;
 end;
 
@@ -341,12 +342,13 @@ begin
    end
    else Result:= false;
    MXList.Free;
    end
    else Result:= false;
    MXList.Free;
+   FTransactionComplete:= false;
 end;
 
 function TRelayer.Greet: boolean;
 { This function reads and checks the relay server's greeting.
 end;
 
 function TRelayer.Greet: boolean;
 { This function reads and checks the relay server's greeting.
+  Then identifies this server with an EHLO.
   Then, if necessary, authenticates at the connected relay server.
   Then, if necessary, authenticates at the connected relay server.
-  Then identifies this server with a HELO.
   The function returns true, if the authentication and the EHLO command were
   successful. }
 var
   The function returns true, if the authentication and the EHLO command were
   successful. }
 var
@@ -360,7 +362,9 @@ begin
    Response.Clear;
    AdministerMassFailure(Result);
    TCP.ReadResponse(Response);
    Response.Clear;
    AdministerMassFailure(Result);
    TCP.ReadResponse(Response);
-   if Response.GetNumericCode = SMTP_R_READY then begin
+
+   { Expect 2xx reply. }
+   if (Response.GetNumericCode div 100) = 2 then begin
 
       TCP.SendCommand(SMTP_C_EHLO, MainServerConfig.Name);
       TCP.ReadResponse(Response);
 
       TCP.SendCommand(SMTP_C_EHLO, MainServerConfig.Name);
       TCP.ReadResponse(Response);
@@ -426,20 +430,32 @@ function TRelayer.SendEnvelope: boolean;
 { Sends the envelope (that is the return-path and the recipient addresses).
   The function returns true, if the MAIL command were successful, and the
   relay server has accepted at least one of the recipient addresses.
 { Sends the envelope (that is the return-path and the recipient addresses).
   The function returns true, if the MAIL command were successful, and the
   relay server has accepted at least one of the recipient addresses.
+  This function returns false if a null reply is read, which is considered
+  as protocol violation.
   This function is aware of the SMTP extension, named PIPELINING. If it's
   supported by the server, we send RCPT commands stuffed, without waiting
   for a response. After all RCPTs are sent, we check all responses. }
 var
   This function is aware of the SMTP extension, named PIPELINING. If it's
   supported by the server, we send RCPT commands stuffed, without waiting
   for a response. After all RCPTs are sent, we check all responses. }
 var
-   i, c: integer; Prms: string;
+   i, c: integer; UltimateFail: boolean; Prms: string;
 
    procedure ProcessRCPTResponse;
    begin
       TCP.ReadResponse(Response);
 
    procedure ProcessRCPTResponse;
    begin
       TCP.ReadResponse(Response);
-      if Response.GetNumericCode = SMTP_R_OK then Inc(c);
+      { If we get an "OK" reply code, we increase the count of sucessful
+        recipients. }
+      if Response.GetNumericCode = SMTP_R_OK then Inc(c)
+      { Response code 0 is non-existent in the SMTP protocol.
+        If we receive something we _perceive_ as 0, then it is likely
+        that something seriously went wrong. MgSMTP shouldn't treat
+        this condition as permanent. }
+      else if Response.GetNumericCode = 0 then UltimateFail:= true;
       Envelope.SetRecipientData(i, Response.GetNumericCode, Response.ReplyText.Text);
    end;
 
 begin
       Envelope.SetRecipientData(i, Response.GetNumericCode, Response.ReplyText.Text);
    end;
 
 begin
+   { The MAIL command is considered the beginning of the transaction. }
+   FTransactionComplete:= false;
+   UltimateFail:= false;
    Response.Clear;
    Prms:= 'FROM:<' + Envelope.ReturnPath + '>';
 
    Response.Clear;
    Prms:= 'FROM:<' + Envelope.ReturnPath + '>';
 
@@ -463,8 +479,16 @@ begin
          for i:= 0 to Envelope.GetNumberOfRecipients - 1 do
             ProcessRCPTResponse;
 
          for i:= 0 to Envelope.GetNumberOfRecipients - 1 do
             ProcessRCPTResponse;
 
-      Result:= c <> 0;
+      Result:= (c <> 0) and (not UltimateFail);
+
+      { If there are no accepted recipients, and no protocol failure is
+        discovered, we can't send the DATA command, and practically we
+        can do nothing more for this transaction. Therefore it is
+        considered complete by protocol. }
+      FTransactionComplete:= (c = 0) and (not UltimateFail);
+
       if not Result then begin
       if not Result then begin
+         { Either way, try to reset SMTP state in case of failure. }
          TCP.SendCommand(SMTP_C_RSET);
          TCP.ReadResponse(Response);
       end;
          TCP.SendCommand(SMTP_C_RSET);
          TCP.ReadResponse(Response);
       end;
@@ -499,7 +523,12 @@ var i: integer;
 begin
    TCP.WriteLn('.');
    TCP.ReadResponse(Response);
 begin
    TCP.WriteLn('.');
    TCP.ReadResponse(Response);
+
+   { Mark the transaction complete, if we have a valid response. }
+   FTransactionComplete:= Response.GetNumericCode <> 0;
+
    for i:= 0 to Envelope.GetNumberOfRecipients - 1 do begin
    for i:= 0 to Envelope.GetNumberOfRecipients - 1 do begin
+      { Set status code for recipients those were accepted in the envelope stage: }
       if Envelope.GetRecipient(i).Data = SMTP_R_OK then
          Envelope.SetRecipientData(i, Response.GetNumericCode, Response.ReplyText.Text);
    end;
       if Envelope.GetRecipient(i).Data = SMTP_R_OK then
          Envelope.SetRecipientData(i, Response.GetNumericCode, Response.ReplyText.Text);
    end;
index 558205d87ff701ee04cdd100eebf566552bbc442..1e4510192f8dd04f97fbf947dd26e0835523d652 100644 (file)
--- a/Spool.pas
+++ b/Spool.pas
@@ -1,6 +1,6 @@
 {
    MegaBrutal's SMTP Server (MgSMTP)
 {
    MegaBrutal's SMTP Server (MgSMTP)
-   Copyright (C) 2010-2014 MegaBrutal
+   Copyright (C) 2010-2015 MegaBrutal
 
    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU Affero General Public License as published by
 
    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU Affero General Public License as published by
@@ -133,7 +133,7 @@ type
       SpoolConfig: TSpoolConfig;
       SpoolFilters: TSpoolFilters;
       function DeliverLocalMessage(SpoolObject: TSpoolObjectReader; MailboxPtr: pointer; ReturnPath, Recipient: string): integer;
       SpoolConfig: TSpoolConfig;
       SpoolFilters: TSpoolFilters;
       function DeliverLocalMessage(SpoolObject: TSpoolObjectReader; MailboxPtr: pointer; ReturnPath, Recipient: string): integer;
-      procedure DeliverRelayMessage(SpoolObject: TSpoolObjectReader; Relayer: TRelayer);
+      function DeliverRelayMessage(SpoolObject: TSpoolObjectReader; Relayer: TRelayer): boolean;
       procedure HandleFailure(SpoolObject: TSpoolObjectReader; IsLocal: boolean; FailEnvelope: TEnvelope; FailedRecipient: TRecipient; AddStatus: integer; FailMsg: string);
       procedure HandleDeliveryResults(SpoolObject: TSpoolObjectReader; IsLocal: boolean; Envelope, FailEnvelope: TEnvelope; AddStatus: integer; FailMsg: string);
       procedure CreateBounceMessage(SourceSpoolObject: TSpoolObjectReader; FailEnvelope: TEnvelope);
       procedure HandleFailure(SpoolObject: TSpoolObjectReader; IsLocal: boolean; FailEnvelope: TEnvelope; FailedRecipient: TRecipient; AddStatus: integer; FailMsg: string);
       procedure HandleDeliveryResults(SpoolObject: TSpoolObjectReader; IsLocal: boolean; Envelope, FailEnvelope: TEnvelope; AddStatus: integer; FailMsg: string);
       procedure CreateBounceMessage(SourceSpoolObject: TSpoolObjectReader; FailEnvelope: TEnvelope);
@@ -662,11 +662,8 @@ begin
             R:= Mailbox^.DeliverMessagePart(LockID, Chunk);
          end;
          if R then begin
             R:= Mailbox^.DeliverMessagePart(LockID, Chunk);
          end;
          if R then begin
-            if Mailbox^.FinishDeliverMessage(LockID) then begin
-               Result:= 0;
-               { It's better to set in Execute. }
-               {SpoolObject.QuickSetDeliveryStatus(Recipient, DS_DELIVERED);}
-            end
+            if Mailbox^.FinishDeliverMessage(LockID) then
+               Result:= 0
             else
                Result:= 4;
          end
             else
                Result:= 4;
          end
@@ -682,7 +679,10 @@ begin
       Result:= 1;
 end;
 
       Result:= 1;
 end;
 
-procedure TDeliveryThread.DeliverRelayMessage(SpoolObject: TSpoolObjectReader; Relayer: TRelayer);
+function TDeliveryThread.DeliverRelayMessage(SpoolObject: TSpoolObjectReader; Relayer: TRelayer): boolean;
+{ Relay message to remote server.
+  Returns true if the transaction went through (doesn't necessarily mean
+  that the message was actually accepted). }
 var Headers, Chunk: TStrings; R: boolean;
 begin
    if Relayer.PrepareSendMessage then begin
 var Headers, Chunk: TStrings; R: boolean;
 begin
    if Relayer.PrepareSendMessage then begin
@@ -690,11 +690,12 @@ begin
       Chunk:= TStringList.Create;
       { Leave a line between the headers and the body. }
       Headers.Add('');
       Chunk:= TStringList.Create;
       { Leave a line between the headers and the body. }
       Headers.Add('');
+
       R:= Relayer.DeliverMessagePart(Headers);
       while (not SpoolObject.IsEOF) and R do begin
       R:= Relayer.DeliverMessagePart(Headers);
       while (not SpoolObject.IsEOF) and R do begin
-         { Maybe constant "32" should be configurable? }
+         { Maybe constant "64" should be configurable? }
          Chunk.Clear;
          Chunk.Clear;
-         SpoolObject.ReadChunk(Chunk, 32);
+         SpoolObject.ReadChunk(Chunk, 64);
          R:= Relayer.DeliverMessagePart(Chunk);
       end;
       if R then begin
          R:= Relayer.DeliverMessagePart(Chunk);
       end;
       if R then begin
@@ -702,9 +703,12 @@ begin
       end
       else
          SpoolObject.SetDeliveryStatus(false, Relayer.Envelope, DS_DELAYED or DS_CONNECTIONFAIL);
       end
       else
          SpoolObject.SetDeliveryStatus(false, Relayer.Envelope, DS_DELAYED or DS_CONNECTIONFAIL);
+
+      Result:= R;
       Chunk.Free;
       Headers.Free;
       Chunk.Free;
       Headers.Free;
-   end;
+   end
+   else Result:= false;
 end;
 
 function TDeliveryThread.NeedSendReport(SpoolObject: TSpoolObject): boolean;
 end;
 
 function TDeliveryThread.NeedSendReport(SpoolObject: TSpoolObject): boolean;
@@ -866,9 +870,10 @@ begin
                         if Relayer.Greet then
                            if Relayer.SendEnvelope then
                               DeliverRelayMessage(SpoolObject, Relayer);
                         if Relayer.Greet then
                            if Relayer.SendEnvelope then
                               DeliverRelayMessage(SpoolObject, Relayer);
+
+                        AssignDeliveryStatusToSMTPCodes(CurrEnv, Relayer.IsTransactionComplete);
                         Relayer.CloseConnection;
                         Relayer.Free;
                         Relayer.CloseConnection;
                         Relayer.Free;
-                        AssignDeliveryStatusToSMTPCodes(CurrEnv);
                         HandleDeliveryResults(SpoolObject, false, CurrEnv, FailEnv, 0, '');
                      end
                      else begin
                         HandleDeliveryResults(SpoolObject, false, CurrEnv, FailEnv, 0, '');
                      end
                      else begin