Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

crypto/tls: Conn.handshakeFn cause memory leak #41987

Closed
For-ACGN opened this issue Oct 15, 2020 · 8 comments
Closed

crypto/tls: Conn.handshakeFn cause memory leak #41987

For-ACGN opened this issue Oct 15, 2020 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@For-ACGN
Copy link

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 toothrot added this to the Backlog milestone Oct 15, 2020
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 15, 2020
@toothrot
Copy link
Contributor

/cc @FiloSottile

@AZ-X
Copy link

AZ-X commented Nov 27, 2020

@For-ACGN have you tested fdecb5c?

@YijinLiu
Copy link

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

@peterrayshen
Copy link

peterrayshen commented Jun 16, 2021 •
edited

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
Copy link
Contributor

@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
Copy link

@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
Copy link
Contributor

Cool, thanks for explaining.

@davecheney
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Jun 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@davecheney @toothrot @gopherbot @YijinLiu @peterrayshen @For-ACGN @AZ-X