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

x/perf: resource leaks from unclosed objects reported by staticmajor #56188

Open
odeke-em opened this issue Oct 13, 2022 · 1 comment
Open

x/perf: resource leaks from unclosed objects reported by staticmajor #56188

odeke-em opened this issue Oct 13, 2022 · 1 comment
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@odeke-em
Copy link
Member

What did you do?

Ran staticmajor within Orijtech Inc, and it produced this manifest of issues

/go/src/golang.org/x/perf/storage/client.go:228:3: leaking resource created on line 224
/go/src/golang.org/x/perf/storage/db/db.go:57:3: leaking resource created on line 55
/go/src/golang.org/x/perf/storage/db/db.go:60:3: leaking resource created on line 55
/go/src/golang.org/x/perf/storage/db/db.go:250:18: leaking resource
/go/src/golang.org/x/perf/storage/db/db.go:232:15: leaking resource
/go/src/golang.org/x/perf/storage/fs/gcs/gcs.go:22:34: leaking resource

in which:

  • there are sql.Statements that are unclosed because they were created like this _, err = tx.Stmt(db.insertUpload).Exec(id, day, num)
  • there is an unclosed sql.DB in case of an error
  • a multipart.Writer is unnecessarily created before an HTTP request is constructed and doesn't error
  • the storage/fs.FS abstraction makes it so that storage/fs/gcs.NewWriter creates a Google Cloud Storage client that never gets closed

the resource leaks are real and all resource leaks except for the storage.Client resource leak can be fixed by this diff

diff --git a/storage/client.go b/storage/client.go
index 12d6ce2..dee1e49 100644
--- a/storage/client.go
+++ b/storage/client.go
@@ -221,12 +221,13 @@ func (c *Client) NewUpload(ctx context.Context) *Upload {
 	hc := c.httpClient()
 
 	pr, pw := io.Pipe()
-	mpw := multipart.NewWriter(pw)
 
 	req, err := http.NewRequest("POST", c.BaseURL+"/upload", pr)
 	if err != nil {
 		return &Upload{err: err}
 	}
+
+	mpw := multipart.NewWriter(pw)
 	req.Header.Set("Content-Type", mpw.FormDataContentType())
 	req.Header.Set("User-Agent", "golang.org/x/perf/storage")
 	errCh := make(chan error)
diff --git a/storage/db/db.go b/storage/db/db.go
index 1d92d24..8ed33d2 100644
--- a/storage/db/db.go
+++ b/storage/db/db.go
@@ -42,11 +42,17 @@ type DB struct {
 // the same as the parameters for sql.Open. Only mysql and sqlite3 are
 // explicitly supported; other database engines will receive MySQL
 // query syntax which may or may not be compatible.
-func OpenSQL(driverName, dataSourceName string) (*DB, error) {
+func OpenSQL(driverName, dataSourceName string) (_ *DB, rerr error) {
 	db, err := sql.Open(driverName, dataSourceName)
 	if err != nil {
 		return nil, err
 	}
+	defer func() {
+		if rerr != nil {
+			db.Close()
+		}
+	}()
+
 	if hook := openHooks[driverName]; hook != nil {
 		if err := hook(db); err != nil {
 			return nil, err
@@ -229,7 +235,10 @@ func (db *DB) NewUpload(ctx context.Context) (*Upload, error) {
 		}
 	}()
 	var lastID string
-	err = tx.Stmt(db.lastUpload).QueryRow().Scan(&lastID)
+	stmt := tx.Stmt(db.lastUpload)
+	err = stmt.QueryRow().Scan(&lastID)
+	stmt.Close()
+
 	switch err {
 	case sql.ErrNoRows:
 	case nil:
@@ -247,7 +256,9 @@ func (db *DB) NewUpload(ctx context.Context) (*Upload, error) {
 
 	id := fmt.Sprintf("%s.%d", day, num)
 
-	_, err = tx.Stmt(db.insertUpload).Exec(id, day, num)
+	stmt = tx.Stmt(db.insertUpload)
+	_, err = stmt.Exec(id, day, num)
+	stmt.Close()
 	if err != nil {
 		return nil, err
 	}

Kind FYI for @kirbyquerby @elias-orijtech @willpoint @jhusdero

@gopherbot gopherbot added this to the Unreleased milestone Oct 13, 2022
@gopherbot
Copy link

Change https://go.dev/cl/442656 mentions this issue: storage: fix resource leaks from unclosed objects found by staticmajor

@joedian joedian added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 14, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: In Progress
Development

No branches or pull requests

3 participants