Close connection if Backend.NewSession() returns an error

[sblinch]

If c.server.Backend.NewSession(c) returns an error at conn.go:232, handleGreet sets c.helo but returns without calling c.setSession(). If the client then issues a MAIL command, handleMail panics when it tries to dereference c.Session():

SMTP session:

EHLO dev.example.org
554 0.7.0 Error during policy check
MAIL FROM:[email protected]
421 4.0.0 Internal server error

Output:

Sep 13 14:43:52 cmail maddy[7298]: goroutine 50 [running]:
Sep 13 14:43:52 cmail maddy[7298]: runtime/debug.Stack()
Sep 13 14:43:52 cmail maddy[7298]:         runtime/debug/stack.go:24 +0x65
Sep 13 14:43:52 cmail maddy[7298]: github.com/emersion/go-smtp.(*Conn).handle.func1()
Sep 13 14:43:52 cmail maddy[7298]:         github.com/emersion/[email protected]/conn.go:103 +0xc5
Sep 13 14:43:52 cmail maddy[7298]: panic({0xf15960, 0x1860b50})
Sep 13 14:43:52 cmail maddy[7298]:         runtime/panic.go:1038 +0x215
Sep 13 14:43:52 cmail maddy[7298]: github.com/emersion/go-smtp.(*Conn).handleMail(0xc00039fad0, {0xc0001e118d, 0x12})
Sep 13 14:43:52 cmail maddy[7298]:         github.com/emersion/[email protected]/conn.go:401 +0xc3f
Sep 13 14:43:52 cmail maddy[7298]: github.com/emersion/go-smtp.(*Conn).handle(0xc00039fad0, {0xc0001e1188, 0xc00039fad0}, {0xc0001e118d, 0x12})
Sep 13 14:43:52 cmail maddy[7298]:         github.com/emersion/[email protected]/conn.go:131 +0x411
Sep 13 14:43:52 cmail maddy[7298]: github.com/emersion/go-smtp.(*Server).handleConn(0xc0003280f0, 0xc00039fad0)
Sep 13 14:43:52 cmail maddy[7298]:         github.com/emersion/[email protected]/server.go:164 +0x2cb
Sep 13 14:43:52 cmail maddy[7298]: created by github.com/emersion/go-smtp.(*Server).Serve
Sep 13 14:43:52 cmail maddy[7298]:         github.com/emersion/[email protected]/server.go:124 +0x169

This trivial patch just closes the connection if NewSession() returns an error, since there's not much else that can be done at that point.

[emersion]

Alternatively, we could move c.helo = domain down right before setSession.

[sblinch]

For sure, and I can amend the PR if that's what you'd prefer. I just wasn't sure if it was appropriate to keep the connection open if NewSession() fails. I suppose it's possible that if the client sent another EHLO, NewSession() could in theory succeed the second time depending on why it failed the first time. And perhaps it's bad protocol etiquette to arbitrarily close the connection on the client. :)

[emersion]

Yeah, I think I'd prefer that. The backend can always close the connection if it prefers that behavior.