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
}
}
Go version
go version go1.23.2 linux/amd64
Output of
go envin your module/workspace: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 anc.Hello()with an empty hostname, which would just send a theEHLOcommand and when that failed aHELO. I was then trying to close the connection to the server withc.Quit()which caused a hang, sinceQuit()will callc.hello()which itself will fail everytime thec.Quit()is called.What did you see happen?
The issue is the
c.hello()call in thec.Quit(). If we sent aHELOand it failed, it will setc.didHelloto true, but it will also set the error toc.helloError. Whenc.Quit()callsc.hello(), it will see thatc.didHellois already set and just returnc.helloError(which was already set with the error we got when the EHLO/HELO failed before. Thereforec.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
QUITto the server, even if we didn't send a successfulHELOorEHLOto 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 eitherc.didHelloisfalseor ifc.didHelloistrueandc.helloErrorisnil. That's how I implemented it in go-mail.See commit: wneessen/go-mail@74fa3f6 (our
net/smtpfork is concurrency safe, therefore the locking. Thenet/smtppatch 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: