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

all: resource leaks due to missing .Close() calls in various conditions #55081

Open
odeke-em opened this issue Sep 15, 2022 · 11 comments
Open

all: resource leaks due to missing .Close() calls in various conditions #55081

odeke-em opened this issue Sep 15, 2022 · 11 comments
Assignees
@odeke-em
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@odeke-em
Copy link
Member

What version of Go are you using (go version)?

$ go version

go version devel go1.20-a813be86df Tue Sep 13 17:43:40 2022 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Irrelevant

What did you do?

Ran a static analyzer developed at Orijtech Inc called "staticmajor" and available on Github actions per https://github.com/marketplace/actions/staticmajor-analyzer and it produced a laundry list of leaking resources like for example

go/src/compress/flate/deflate.go

Lines 665 to 670 in 972870d

func NewWriter(w io.Writer, level int) (*Writer, error) {
var dw Writer
if err := dw.d.init(w, level); err != nil {
return nil, err
}
return &dw, nil

in which the return on line 668 doesn't invoke dw.Close() which leaks the Writer

or

go/src/encoding/pem/pem.go

Lines 291 to 294 in 972870d

b64 := base64.NewEncoder(base64.StdEncoding, &breaker)
if _, err := b64.Write(b.Bytes); err != nil {
return err
}

in which the return on line 293 doesn't invoke b64.Close()

What did you expect to see?

No resource leaks

What did you see instead?

The laundry list is

/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/compress/flate/deflate.go:668:3: leaking resource created on line 0
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/archive/zip/reader.go:77:3: leaking resource created on line 0
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/compress/gzip/gunzip.go:95:3: leaking resource created on line 0
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/compress/zlib/reader.go:86:3: leaking resource created on line 0
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/compress/zlib/writer.go:132:42: leaking resource
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/encoding/pem/pem.go:293:3: leaking resource created on line 291
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/net/sock_posix.go:65:5: leaking resource created on line 27
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/net/sock_posix.go:67:4: leaking resource created on line 27
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/net/sock_posix.go:72:3: leaking resource created on line 27
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/net/sock_posix.go:74:2: leaking resource created on line 27
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/net/udpsock_posix.go:235:4: leaking resource created on line 226
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/crypto/tls/tls.go:159:3: leaking resource created on line 156
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/debug/macho/file.go:227:3: leaking resource created on line 0
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/debug/macho/file.go:239:3: leaking resource created on line 0
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/debug/pe/file.go:70:3: leaking resource created on line 0
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/debug/pe/file.go:78:4: leaking resource created on line 0
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/xcoff/ar.go:120:3: leaking resource created on line 0
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/xcoff/ar.go:122:3: leaking resource created on line 0
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/encoding/xml/marshal.go:84:3: leaking resource created on line 82
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/encoding/xml/marshal.go:135:3: leaking resource created on line 132
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/go/build/build.go:1170:5: leaking resource created on line 1159
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/go/internal/gccgoimporter/ar.go:119:24: leaking resource
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/go/internal/gccgoimporter/ar.go:145:4: leaking resource created on line 139
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/go/internal/gccgoimporter/gccgoinstallation.go:40:3: leaking resource created on line 33
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/fuzz/sys_posix.go:66:3: leaking resource created on line 61
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/fuzz/sys_posix.go:66:3: leaking resource created on line 62
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/fuzz/sys_posix.go:66:3: leaking resource created on line 63
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/fuzz/sys_posix.go:70:3: leaking resource created on line 61
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/fuzz/sys_posix.go:70:3: leaking resource created on line 62
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/fuzz/sys_posix.go:70:3: leaking resource created on line 63
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/fuzz/sys_posix.go:75:3: leaking resource created on line 61
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/fuzz/sys_posix.go:75:3: leaking resource created on line 62
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/trace/parser.go:889:3: leaking resource created on line 882
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/trace/parser.go:893:3: leaking resource created on line 882
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/internal/trace/parser.go:893:3: leaking resource created on line 887
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/net/http/cgi/host.go:249:3: leaking resource created on line 240
/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/net/smtp/smtp.go:369:3: leaking resource created on line 363

Just an FYI to @eliasnaur @kirbyquerby @willpoint @jhusdero
@odeke-em odeke-em added this to the Go1.20 milestone Sep 15, 2022
@odeke-em odeke-em self-assigned this Sep 15, 2022
@gopherbot
Copy link

Change https://go.dev/cl/430997 mentions this issue: all: fix resource leaks

@cuonglm
Copy link
Member

cuonglm commented Sep 15, 2022 •
edited

@odeke-em I think the flag for base64 encoder seems wrong, calling Close just to make sure partial data will be written, the encoder can still be gc-ed even without calling Close:

package main

import (
	"encoding/base64"
	"io"
	"io/ioutil"
	"runtime"
)

func main() {
	ch := make(chan struct{})
	done := make(chan struct{})
	go func() {
		defer close(done)
		input := []byte("foo\x00bar")
		encoder := base64.NewEncoder(base64.StdEncoding, ioutil.Discard)
		runtime.SetFinalizer(encoder, func(io.WriteCloser) {
			defer close(ch)
		})
		encoder.Write(input)
	}()
	<-done
	runtime.GC()
	<-ch
	println("encoder was gc-ed without calling Close()")
}

@rittneje
Copy link

@cuonglm While some of these may be false positives, the fact that the encoder can be garbage collected does not demonstrate the lack of a leak. If for example NewEncoder opened a file descriptor (it doesn't), the garbage collector would not know to close it and we would leak OS resources.

Also, even when there is a proper finalizer set (as is the case for *os.File), neglecting to call Close is still sort of a leak, as the OS resources remain allocated until the garbage collector happens to run, which could cause issues (e.g., running out of fds).

@cuonglm
Copy link
Member

cuonglm commented Sep 15, 2022

@rittneje Yeah, that's why I only describe specifically for base64 encoder. The example demonstrate that we don't leak the writer.

@rittneje
Copy link

My point is that you can really only conclude there is no leak at all by examining the source code of NewWriter. Just showing that the pointer gets garbage collected does not mean much in a vacuum.

@cuonglm
Copy link
Member

cuonglm commented Sep 15, 2022

My point is that you can really only conclude there is no leak at all by examining the source code of NewWriter. Just showing that the pointer gets garbage collected does not mean much in a vacuum.

Not sure why not mean much. You seem to describe the fd leaks case, while my example above demonstrate that there's no memory leak in case of base64 encoder because the encoder was gc-ed.

@rittneje
Copy link

there's no memory leak in case of base64 encoder because the encoder was gc-ed

Even this is contingent upon the implementation of NewEncoder. Here is a contrived counter-example. https://go.dev/play/p/Et_0jcHgeh5

@cuonglm
Copy link
Member

cuonglm commented Sep 15, 2022

there's no memory leak in case of base64 encoder because the encoder was gc-ed

Even this is contingent upon the implementation of NewEncoder. Here is a contrived counter-example. https://go.dev/play/p/Et_0jcHgeh5

Not sure how can it be counter-example:

  • Your Close modify allocated slice, so comparing its length does not mean anything
  • Even you shrink allocated length in Close, the backing array of allocated slice is still there, and your allocated slice still take up the same memory, whether you call Close or not.

@rittneje
Copy link

The []byte that was created in NewFoo cannot be garbage collected as long as there is a reference to it from the global variable. And the only way to remove that reference is by calling Close. The garbage collector cannot do it.

@mvdan mvdan mentioned this issue Sep 15, 2022
Open
@cuonglm
Copy link
Member

cuonglm commented Sep 15, 2022

The []byte that was created in NewFoo cannot be garbage collected as long as there is a reference to it from the global variable. And the only way to remove that reference is by calling Close. The garbage collector cannot do it.

Of course the garbage collector cannot do anything, and your example is different from the original demonstration with the base64 encoder. I just mean showing the length of the global slice does not mean anything. If you remove the reference without modifying the global slice length, they're still gc-ed.

@ianlancetaylor
Copy link
Contributor

I tend to agree that we should not worry about calling Close on the value returned by base64.NewEncoder. The NewEncoder function documents that Close is needed to flush any partially written blocks, but there are no other associated resources. I think that if we were writing this code today we would not call that method Close.

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 19, 2022
@gopherbot gopherbot modified the milestones: Go1.20, Go1.21 Feb 1, 2023
@gopherbot gopherbot modified the milestones: Go1.21, Go1.22 Aug 8, 2023
@odeke-em odeke-em modified the milestones: Go1.22, Unreleased Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@odeke-em odeke-em

Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@ianlancetaylor @cuonglm @odeke-em @gopherbot @seankhliao @rittneje