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 ```go // 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 ```go // 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. ```go // 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 https://github.com/golang/go/commit/fdecb5c5b46a3f0b8f299d9069d428c656576dcb?

[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.