crypto/tls: Conn.handshakeFn cause memory leak

[For-ACGN]

What version of Go are you using (go version)?

$ go version
go version go1.15.3 windows/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
windows 10 1909, amd64

What did you do?

check tls.Conn is destroyed(object unreachable)

What did you expect to see?

destroyed(object unreachable)

What did you see instead?

not destroyed(object reachable)

test code

// destroyed is used to check object is destroyed.
func destroyed(object interface{}) bool {
	destroyed := make(chan struct{})
	runtime.SetFinalizer(object, func(interface{}) {
		close(destroyed)
	})
	// total 3 seconds
	timer := time.NewTimer(10 * time.Millisecond)
	defer timer.Stop()
	for i := 0; i < 300; i++ {
		timer.Reset(10 * time.Millisecond)
		runtime.GC()
		select {
		case <-destroyed:
			return true
		case <-timer.C:
		}
	}
	return false
}


func TestTLSConn(t *testing.T) {
	gm := testsuite.MarkGoroutines(t)
	defer gm.Compare()

	serverCfg, clientCfg := testsuite.TLSConfigPair(t, "127.0.0.1")
	const network = "tcp"

	listener, err := tls.Listen(network, "127.0.0.1:0", serverCfg)
	require.NoError(t, err)
	address := listener.Addr().String()
	go func() {
		conn, err := listener.Accept()
		require.NoError(t, err)

		_, err = conn.Write([]byte("data"))
		require.NoError(t, err)

		time.Sleep(time.Second)

		err = conn.Close()
		require.NoError(t, err)
	}()

	conn, err := tls.Dial(network, address, clientCfg)
	require.NoError(t, err)

	buf := make([]byte, 4)
	_, err = io.ReadFull(conn, buf)
	require.NoError(t, err)

	err = conn.Close()
	require.NoError(t, err)

	require.True(t, destroyed(conn)) // here
}

tls.Conn is not destroyed

// crypto/tls/conn.go:

// A Conn represents a secured connection.
// It implements the net.Conn interface.
type Conn struct {
	// constant
	conn        net.Conn
	isClient    bool
	handshakeFn func() error // (*Conn).clientHandshake or serverHandshake
	// handshakeFn will reference self
	// ...
}

// crypto/tls/tls.go:

// Server returns a new TLS server side connection
// using conn as the underlying transport.
// The configuration config must be non-nil and must include
// at least one certificate or else set GetCertificate.
func Server(conn net.Conn, config *Config) *Conn {
	c := &Conn{
		conn:   conn,
		config: config,
	}
	c.handshakeFn = c.serverHandshake // reference self
	return c
}

// Client returns a new TLS client side connection
// using conn as the underlying transport.
// The config cannot be nil: users must set either ServerName or
// InsecureSkipVerify in the config.
func Client(conn net.Conn, config *Config) *Conn {
	c := &Conn{
		conn:     conn,
		config:   config,
		isClient: true,
	}
	c.handshakeFn = c.clientHandshake // reference self
	return c
}

fix it that don't use handshakeFn.

// crypto/tls/conn.go:

func (c *Conn) Handshake() error {
	c.handshakeMutex.Lock()
	defer c.handshakeMutex.Unlock()

	if err := c.handshakeErr; err != nil {
		return err
	}
	if c.handshakeComplete() {
		return nil
	}

	c.in.Lock()
	defer c.in.Unlock()

	// use it
	if c.isClient {
		c.handshakeErr = c.clientHandshake()
	} else {
		c.handshakeErr = c.serverHandshake()
	}
	// c.handshakeErr = c.handshakeFn()
	if c.handshakeErr == nil {
		c.handshakes++
	} else {
		// If an error occurred during the handshake try to flush the
		// alert that might be left in the buffer.
		c.flush()
	}

	if c.handshakeErr == nil && !c.handshakeComplete() {
		c.handshakeErr = errors.New("tls: internal error: handshake should have had a result")
	}

	return c.handshakeErr
}

fix it and TestTLSConn is passed.

[toothrot]

/cc @FiloSottile

[AZ-X]

@For-ACGN have you tested fdecb5c?

[YijinLiu]

The change from @For-ACGN is simple and IMHO better than the original version. Any reason not to adopt it?

[peterrayshen]

Any update on this @FiloSottile @toothrot ? Created another contrived example that seems to reproduce the memory leak. (Edit: Nevermind, once you remove the finalizer the memory stays constant - specific to my example )

main.go

package main

import (
	"crypto/tls"
	"fmt"
	"log"
	_ "net/http/pprof"
	"os"
	"os/signal"
	"runtime"
	"syscall"
	"time"
)

func finalizer(_ interface{}) {
	fmt.Println("finalizer called")
}

func main() {
	// setup interrupt handler
	c := make(chan os.Signal)
	signal.Notify(c, os.Interrupt, syscall.SIGTERM)

	for i := 0; i < 100; i++ {
		go func() {
			for {
				tlsConnectThenCloseAfterWait()
			}
		}()
	}

	<-c
	os.Exit(1)

}

func tlsConnectThenCloseAfterWait() {
	conn, err := tls.Dial("tcp", "mail.google.com:443", &tls.Config{})
	if err != nil {
		log.Fatalln("failed to connect: " + err.Error())
	}
	defer func() {
		err := conn.Close()
		if err != nil {
			log.Fatalln("closing conn failed")
		}
	}()

	runtime.SetFinalizer(conn, finalizer)
	conn.Write([]byte("hello how are you"))

	timer := time.NewTimer(time.Second)
	<-timer.C
}

Output of GODEBUG=gctrace=1 ./main

gc 1 @0.088s 1%: 0+9.0+0 ms clock, 0+12/11/0+0 ms cpu, 4->5->1 MB, 5 MB goal, 12 P
gc 2 @0.102s 3%: 0+4.9+0.99 ms clock, 0+5.9/6.0/0+11 ms cpu, 4->4->1 MB, 5 MB goal, 12 P
gc 3 @0.114s 5%: 0+4.9+1.0 ms clock, 0+3.9/10/2.9+12 ms cpu, 4->4->2 MB, 5 MB goal, 12 P
gc 4 @0.171s 4%: 0+5.0+0 ms clock, 0+1.0/9.9/0+0 ms cpu, 4->5->3 MB, 5 MB goal, 12 P
gc 5 @0.196s 4%: 0+5.9+0 ms clock, 0+2.9/9.9/0+0 ms cpu, 5->7->3 MB, 6 MB goal, 12 P
gc 6 @0.352s 2%: 1.0+2.0+0 ms clock, 12+0/1.9/1.9+0 ms cpu, 6->7->4 MB, 7 MB goal, 12 P
gc 7 @0.365s 2%: 0.99+3.0+0 ms clock, 11+3.0/5.0/0+0 ms cpu, 7->8->5 MB, 8 MB goal, 12 P
gc 8 @0.399s 3%: 0+13+0 ms clock, 0+18/29/1.0+0 ms cpu, 9->11->6 MB, 10 MB goal, 12 P
gc 9 @1.278s 1%: 1.0+28+0 ms clock, 12+9.9/53/0+0 ms cpu, 10->13->9 MB, 13 MB goal, 12 P
gc 10 @1.433s 2%: 1.0+22+0 ms clock, 12+45/55/1.0+0 ms cpu, 14->16->9 MB, 18 MB goal, 12 P
gc 11 @1.534s 2%: 0+6.0+0 ms clock, 0+4.0/15/3.0+0 ms cpu, 16->17->11 MB, 19 MB goal, 12 P
gc 12 @2.479s 1%: 0+3.0+0 ms clock, 0+0/6.0/18+0 ms cpu, 20->20->12 MB, 22 MB goal, 12 P
gc 13 @2.656s 1%: 1.0+10+0 ms clock, 12+3.0/30/4.9+0 ms cpu, 23->25->16 MB, 25 MB goal, 12 P
gc 14 @3.737s 1%: 0+6.0+0 ms clock, 0+3.0/18/9.0+0 ms cpu, 31->33->20 MB, 33 MB goal, 12 P
gc 15 @4.830s 0%: 0+5.9+0 ms clock, 0+5.0/13/16+0 ms cpu, 39->40->25 MB, 41 MB goal, 12 P
gc 16 @6.733s 0%: 0.99+16+0.99 ms clock, 11+7.9/47/80+11 ms cpu, 50->50->32 MB, 51 MB goal, 12 P
gc 17 @8.140s 0%: 0.99+21+0 ms clock, 11+3.0/59/125+0 ms cpu, 64->64->42 MB, 65 MB goal, 12 P
gc 18 @11.168s 0%: 1.0+28+0 ms clock, 12+24/78/97+0 ms cpu, 82->82->54 MB, 84 MB goal, 12 P
gc 19 @14.433s 0%: 0.99+27+0 ms clock, 11+9.0/74/146+0 ms cpu, 106->106->70 MB, 108 MB goal, 12 P
gc 20 @18.883s 0%: 0+47+0 ms clock, 0+6.0/133/211+0 ms cpu, 137->138->91 MB, 140 MB goal, 12 P
gc 21 @24.437s 0%: 0.99+30+0.99 ms clock, 11+15/91/101+11 ms cpu, 177->178->118 MB, 182 MB goal, 12 P
gc 22 @31.872s 0%: 1.0+105+0 ms clock, 12+60/317/256+0 ms cpu, 230->233->155 MB, 236 MB goal, 12 P
gc 23 @41.705s 0%: 0+101+0 ms clock, 0+15/283/549+0 ms cpu, 302->303->200 MB, 310 MB goal, 12 P
gc 24 @54.302s 0%: 0+92+0 ms clock, 0+9.0/278/472+0 ms cpu, 390->392->260 MB, 400 MB goal, 12 P
gc 25 @70.777s 0%: 0+38+0 ms clock, 0+4.9/113/321+0 ms cpu, 508->508->337 MB, 521 MB goal, 12 P
gc 26 @92.203s 0%: 0+108+0 ms clock, 0+57/326/391+0 ms cpu, 658->662->442 MB, 675 MB goal, 12 P
gc 27 @120.781s 0%: 2.0+99+0 ms clock, 24+11/292/529+0 ms cpu, 862->864->574 MB, 884 MB goal, 12 P

Note that the finalizer is never called, and the memory keeps on growing.

go version
go version go1.15.8 windows/amd64

Edit: Nevermind, once you remove the finalizer the memory stays constant.

[davecheney]

@peterrayshen thank you for the code sample. Could you clarify a little, you say that if you don't wrap a finaliser around the connection the memory leak is gone? This is good news, but also, what does this have to do with the crypto/tls package?

[peterrayshen]

@davecheney Thanks for the response - clarification answered here: https://stackoverflow.com/questions/68011306/golang-crypto-tls-memory-leak. Yeah, the leak in my code snippet doesn't have anything to do with the crypto/tls package, although I thought it did beforehand. I can delete my post to avoid sidetracking the thread/confusing readers - let me know.

[davecheney]

Cool, thanks for explaining.

[davecheney]

Given the lack of response from the OP I will close this issue for now.