Fix two issues in handling of DSN messages in SMTP pipeline and checks

First issue: check.spf CheckBody deadlocks if CheckConnection skipped
the message due to it being locally generated (the case for DSNs).

Second issue: msgpipeline does not call CheckConnection at all
if MAIL FROM is an empty string (which is also the case for DSNs).

tests/issue327_test.go is added based on symptoms from the original
bug report.
See foxcpp#237.

internal/check/spf/spf.go

@@ -119,6 +119,8 @@ type state struct {
 	msgMeta  *module.MsgMetadata
 	spfFetch chan spfRes
 	log      log.Logger
+
+	skip bool
 }
 
 func (c *Check) CheckStateForMsg(ctx context.Context, msgMeta *module.MsgMetadata) (module.CheckState, error) {
@@ -298,18 +300,21 @@ func (s *state) CheckConnection(ctx context.Context) module.CheckResult {
 	defer trace.StartRegion(ctx, "check.spf/CheckConnection").End()
 
 	if s.msgMeta.Conn == nil {
+		s.skip = true
 		s.log.Println("locally generated message, skipping")
 		return module.CheckResult{}
 	}
 
 	ip, ok := s.msgMeta.Conn.RemoteAddr.(*net.TCPAddr)
 	if !ok {
+		s.skip = true
 		s.log.Println("non-IP SrcAddr")
 		return module.CheckResult{}
 	}
 
 	mailFrom, err := prepareMailFrom(s.msgMeta.OriginalFrom)
 	if err != nil {
+		s.skip = true
 		return module.CheckResult{
 			Reason: err,
 			Reject: true,
@@ -356,7 +361,7 @@ func (s *state) CheckRcpt(ctx context.Context, rcptTo string) module.CheckResult
 }
 
 func (s *state) CheckBody(ctx context.Context, header textproto.Header, body buffer.Buffer) module.CheckResult {
-	if s.c.enforceEarly {
+	if s.c.enforceEarly || s.skip {
 		// Already applied in CheckConnection.
 		return module.CheckResult{}
 	}

internal/msgpipeline/check_runner.go

@@ -35,8 +35,9 @@ import (
 // checkRunner runs groups of checks, collects and merges results.
 // It also makes sure that each check gets only one state object created.
 type checkRunner struct {
-	msgMeta  *module.MsgMetadata
-	mailFrom string
+	msgMeta          *module.MsgMetadata
+	mailFrom         string
+	mailFromReceived bool
 
 	checkedRcpts         []string
 	checkedRcptsPerCheck map[module.CheckState]map[string]struct{}
@@ -102,7 +103,7 @@ func (cr *checkRunner) checkStates(ctx context.Context, checks []module.Check) (
 	//
 	// Done outside of check loop above to make sure we can run these for multiple
 	// checks in parallel.
-	if cr.mailFrom != "" {
+	if cr.mailFromReceived {
 		err := cr.runAndMergeResults(newStates, func(s module.CheckState) module.CheckResult {
 			res := s.CheckConnection(ctx)
 			return res
@@ -232,6 +233,7 @@ func (cr *checkRunner) runAndMergeResults(states []module.CheckState, runner fun
 
 func (cr *checkRunner) checkConnSender(ctx context.Context, checks []module.Check, mailFrom string) error {
 	cr.mailFrom = mailFrom
+	cr.mailFromReceived = true
 
 	// checkStates will run CheckConnection and CheckSender.
 	_, err := cr.checkStates(ctx, checks)

tests/issue327_test.go

@@ -0,0 +1,90 @@
+//+build integration
+
+/*
+Maddy Mail Server - Composable all-in-one email server.
+Copyright © 2019-2020 Max Mazurov <[email protected]>, Maddy Mail Server contributors
+
+This program is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 3 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see <https://www.gnu.org/licenses/>.
+*/
+
+package tests_test
+
+import (
+	"strconv"
+	"testing"
+	"time"
+
+	"github.com/foxcpp/maddy/internal/testutils"
+	"github.com/foxcpp/maddy/tests"
+)
+
+func TestIssue327(tt *testing.T) {
+	tt.Parallel()
+	t := tests.NewT(tt)
+	t.DNS(nil)
+	t.Port("smtp")
+	tgtPort := t.Port("target")
+	t.Config(`
+		hostname mx.maddy.test
+		tls off
+		smtp tcp://127.0.0.1:{env:TEST_PORT_smtp} {
+			deliver_to queue outbound_queue {
+				target remote { } # it will fail
+				autogenerated_msg_domain maddy.test
+				bounce {
+					check {
+						spf
+					}
+					deliver_to lmtp tcp://127.0.0.1:{env:TEST_PORT_target}
+				}
+			}
+		}`)
+	t.Run(1)
+	defer t.Close()
+
+	be, s := testutils.SMTPServer(tt, "127.0.0.1:"+strconv.Itoa(int(tgtPort)))
+	s.LMTP = true
+	be.LMTPDataErr = []error{nil, nil}
+	defer s.Close()
+
+	c := t.Conn("smtp")
+	defer c.Close()
+	c.SMTPNegotation("client.maddy.test", nil, nil)
+	c.Writeln("MAIL FROM:<[email protected]>")
+	c.ExpectPattern("250 *")
+	c.Writeln("RCPT TO:<[email protected]>")
+	c.ExpectPattern("250 *")
+	c.Writeln("DATA")
+	c.ExpectPattern("354 *")
+	c.Writeln("From: <[email protected]>")
+	c.Writeln("To: <[email protected]>")
+	c.Writeln("Subject: Hello!")
+	c.Writeln("")
+	c.Writeln("Hello!")
+	c.Writeln(".")
+	c.ExpectPattern("250 2.0.0 OK: queued")
+	c.Writeln("QUIT")
+	c.ExpectPattern("221 *")
+
+	for i := 0; i < 5; i++ {
+		time.Sleep(1 * time.Second)
+		if len(be.Messages) != 0 {
+			break
+		}
+	}
+
+	if len(be.Messages) != 1 {
+		t.Fatal("No DSN sent?", len(be.Messages))
+	}
+}