database/sql: memory leaks in Stmt.removeClosedStmtLocked

[apocelipes]

Go version

go devel go1.23-8f7df2256e

Output of go env in your module/workspace:

set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\apocelipes\AppData\Local\go-build
set GOENV=C:\Users\apocelipes\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\apocelipes\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\apocelipes\go
set GOPRIVATE=
set GOPROXY=
set GOROOT=C:/Users/apocelipes/work/go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=C:\Users\apocelipes\work\go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=devel go1.23-8f7df2256e Tue Mar 19 16:19:26 2024 +0000
set GODEBUG=
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=0
set GOMOD=C:\Users\apocelipes\work\go\src\go.mod
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=C:\Users\apocelipes\AppData\Local\Temp\go-build136483040=/tmp/go-build -gno-record-gcc-switches

What did you do?

I was reading through the source code of database/sql and I found some potential memory leaks.

What did you see happen?

In the method "Stmt.removeClosedStmtLocked", old elements are not cleared after slice shrinking:

func (s *Stmt) removeClosedStmtLocked() {
	...

	s.db.mu.Lock()
	for i := 0; i < len(s.css); i++ {
		if s.css[i].dc.dbmuClosed {
			s.css[i] = s.css[len(s.css)-1]
			s.css = s.css[:len(s.css)-1]  // s.css[len(s.css)-1] leaked
			i--
		}
	}
	s.db.mu.Unlock()
	...
}

The code is here.

Since "s.css" is a slice of ”connStmt“ and ”connStmt“ contains two pointer fields, if we don't set the element removed from "s.css" to a zero value, a potential memory leak will occur.

What did you expect to see?

We should zero out the obsolete elements or just use "slices.DeleteFunc" to avoid leaks and simplify the code.

func (s *Stmt) removeClosedStmtLocked() {
	t := len(s.css)/2 + 1
	if t > 10 {
		t = 10
	}
	dbClosed := s.db.numClosed.Load()
	if dbClosed-s.lastNumClosed < uint64(t) {
		return
	}

	s.db.mu.Lock()
	s.css = slices.DeleteFunc(s.css, func (cs connStmt) bool { return cs.dc.dbmuClosed })
	s.db.mu.Unlock()
	s.lastNumClosed = dbClosed
}

Using "slices.DeleteFunc" will result in more data copies, but I don't think it will have much impact on "connStmt" (It's small).

If you think this need to be fixed, I'm happy to contribute a related pull request.

[odeke-em]

Thank you @apocelipes, for the discovery and for the investigation! Yes please, welcome to the Go project and we would greatly appreciate it if you could send a change through!

However, I do encourage the much simpler and cheaper approach of inserting a nil at the end of the slice and adding a comment

s.css[i] = s.css[len(c.ss)-1]
// Zero the last element before shrinking, for GC.
s.css[len(s.css)-1] = nil
s.css = s.css[:len(s.css)-1]

[gopherbot]

Change https://go.dev/cl/572956 mentions this issue: database/sql: fix memory leaks in Stmt.removeClosedStmtLocked