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.

Sign up for GitHub

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

database/sql: memory leaks in Stmt.removeClosedStmtLocked #66410

Closed
apocelipes opened this issue Mar 19, 2024 · 2 comments
Closed

database/sql: memory leaks in Stmt.removeClosedStmtLocked #66410

apocelipes opened this issue Mar 19, 2024 · 2 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@apocelipes
Copy link
Contributor

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

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]

@odeke-em odeke-em added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 19, 2024
@odeke-em odeke-em added this to the Unreleased milestone Mar 19, 2024
apocelipes added a commit to apocelipes/go that referenced this issue Mar 20, 2024
@apocelipes
database/sql: fix memory leaks in Stmt.removeClosedStmtLocked
cbaf396 
Zero out elements before shrinking the slice to avoid memory leaks.

Fixes golang#66410.
@apocelipes apocelipes mentioned this issue Mar 20, 2024
Closed
@gopherbot
Copy link

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

apocelipes added a commit to apocelipes/go that referenced this issue Mar 20, 2024
@apocelipes
database/sql: fix memory leaks in Stmt.removeClosedStmtLocked
bf20b32 
Zero out elements before shrinking the slice to avoid memory leaks.

Fixes golang#66410
apocelipes added a commit to apocelipes/go that referenced this issue Mar 20, 2024
@apocelipes
database/sql: fix memory leaks in Stmt.removeClosedStmtLocked
a87ab9e 
Zero out elements before shrinking the slice to avoid memory leaks.

Fixes golang#66410.
apocelipes added a commit to apocelipes/go that referenced this issue Mar 20, 2024
@apocelipes
database/sql: fix memory leaks in Stmt.removeClosedStmtLocked
9a61a59 
Zero out elements before shrinking the slice to avoid memory leaks.

Fixes golang#66410
apocelipes added a commit to apocelipes/go that referenced this issue Mar 20, 2024
@apocelipes
database/sql: fix memory leaks in Stmt.removeClosedStmtLocked
b15586e 
Zero out elements before shrinking the slice to avoid memory leaks.

Fixes golang#66410
bradfitz pushed a commit to tailscale/go that referenced this issue Apr 3, 2024
@apocelipes @bradfitz
database/sql: fix memory leaks in Stmt.removeClosedStmtLocked
48d7185 
Zero out elements before shrinking the slice to avoid memory leaks.

Fixes golang#66410

Change-Id: I8f64c21455761f7f7c8b6fee0b6450b98f691d91
GitHub-Last-Rev: b15586e
GitHub-Pull-Request: golang#66419
Reviewed-on: https://go-review.googlesource.com/c/go/+/572956
TryBot-Result: Gopher Robot <gobot@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
@bradfitz bradfitz mentioned this issue Apr 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

Successfully merging a pull request may close this issue.

database/sql: fix memory leaks in Stmt.removeClosedStmtLocked apocelipes/go
3 participants
@odeke-em @gopherbot @apocelipes