net/smtp: Quit() will continously fail if a previous EHLO/HELO failed #70011
Description
Go version
go version go1.23.2 linux/amd64
Output of go env in your module/workspace:
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/xxx/.cache/go-build'
GOENV='/home/xxx/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/xxx/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/xxx/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.2'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/xxx/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/xxxx/go/src/go-mail/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2680745108=/tmp/go-build -gno-record-gcc-switches'What did you do?
While working on some test in go-mail (we forked net/smtp) I was working on a test to check a failed HELO/EHLO. I was sending an c.Hello() with an empty hostname, which would just send a the EHLO command and when that failed a HELO. I was then trying to close the connection to the server with c.Quit() which caused a hang, since Quit() will call c.hello() which itself will fail everytime the c.Quit() is called.
What did you see happen?
The issue is the c.hello() call in the c.Quit(). If we sent a HELO and it failed, it will set c.didHello to true, but it will also set the error to c.helloError. When c.Quit() calls c.hello(), it will see that c.didHellois already set and just return c.helloError (which was already set with the error we got when the EHLO/HELO failed before. Therefore c.Quit() will fail with that error from the HELO that we've seen before and we are basically stuck. We will not be able to send a QUIT to the server to close the connection gracefully.
What did you expect to see?
I think we should always be able to send a QUIT to the server, even if we didn't send a successful HELO or EHLO to the server. That way we are able close the connection gracefully even if we error'd.
I suggest that the c.hello() is only called when either c.didHello is false or if c.didHello is true and c.helloError is nil. That's how I implemented it in go-mail.
See commit: wneessen/go-mail@74fa3f6 (our net/smtp fork is concurrency safe, therefore the locking. The net/smtp patch would not need this.
This would probably suffice:
At https://cs.opensource.google/go/go/+/refs/tags/go1.23.2:src/net/smtp/smtp.go;l=83
Make it:
if !c.didHello || c.helloError == nil {
if err := c.hello(); err != nil {
return err
}
}