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

net: Buffers.WriteTo is prone to memory leaks #45163

Closed
jwebb opened this issue Mar 22, 2021 · 10 comments
Closed

net: Buffers.WriteTo is prone to memory leaks #45163

jwebb opened this issue Mar 22, 2021 · 10 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@jwebb
Copy link

jwebb commented Mar 22, 2021

Buffers.WriteTo modifies its receiver (inside consume), leaving part of the backing array inaccessible. But it can still contain references that pin potentially large buffers into memory, so if the client code continues to hold the remainder of the slice for further use, it may have much higher memory consumption than expected.

The fix is simply to set (v*)[0] to nil before advancing.
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Mar 22, 2021
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Mar 22, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/303829 mentions this issue: net: fix Buffers.WriteTo memory leaks

@cbandy
Copy link

cbandy commented Dec 1, 2021

It looks to me like the writev implementation manipulates the net.Buffers value directly. Should these two implementations be the same?

go/src/net/net.go

Lines 731 to 742 in f58c78a

func (v *Buffers) consume(n int64) {
for len(*v) > 0 {
ln0 := int64(len((*v)[0]))
if ln0 > n {
(*v)[0] = (*v)[0][n:]
return
}
n -= ln0
(*v)[0] = nil
*v = (*v)[1:]
}
}

go/src/internal/poll/fd.go

Lines 69 to 79 in f58c78a

func consume(v *[][]byte, n int64) {
for len(*v) > 0 {
ln0 := int64(len((*v)[0]))
if ln0 > n {
(*v)[0] = (*v)[0][n:]
return
}
n -= ln0
*v = (*v)[1:]
}
}

@ianlancetaylor
Copy link
Member

@cbandy Yes, they should. Want to send a patch for 1.19?

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/373374 mentions this issue: internal/poll: clear completed Buffers to permit earlier collection

@martin-sucha
Copy link
Contributor

Isn't this a breaking change?

For example code like

https://github.com/mailru/easyjson/blob/11c9d7f52fd019df40f13aeecd28f11d941be9e3/buffer/pool.go#L171-L180

and

https://github.com/wubbalubbaaa/easyNet/blob/7d610c40038c927c38f2a4c3990db2e0c37cae9f/conn_std.go#L114

does not work correctly anymore since Go 1.17 because it expects the underlying array to not be modified.

I have tried searching for usages of net.Buffers on GitHub. Most of the time, the net.Buffers were allocated temporarily just before calling WriteTo and then not referenced anymore. In that case no leak occurred before this change as the backing memory was not reachable anymore. Other occurrences like

https://github.com/easybi/v2ray-core/blob/75e78fa008e4a571a3108761750d5ed9ca699a57/common/buf/writer.go#L43

cleared the memory already. Other uses tried to cache the underlying buffers, so it is not clear that zeroing the elements in the underlying array is better than not touching them.

After the change, it is also not possible to reuse the net.Buffers underlying data to write to multiple writers without allocating a copy of the net.Buffers (e.g. to retry on another connection), but I haven't found any usages that would do so.

@martin-sucha martin-sucha mentioned this issue Feb 20, 2022
Open
@martin-sucha
Copy link
Contributor

I also forgot to mention

https://github.com/kiwicom/gocql/blob/354a9342c24f61f448e0eff351eda4438f8d7b55/conn.go#L1031-L1033

which is another instance where not modifying the backing array would be helpful, although this piece of code could be rewritten to not need the underlying array.

@ianlancetaylor
Copy link
Member

Thanks. That's interesting.

This change already went out in the Go 1.17 release. So the question now is whether we should roll it back for 1.18, or just document it. Given that it has already gone out, and given that the (Buffers).WriteTo method has always changed its receiver, my inclination is to document it.

@cbandy
Copy link

cbandy commented Feb 22, 2022

does not work correctly anymore since Go 1.17 because it expects the underlying array to not be modified.

My understanding is that these examples were incorrect before Go 1.17. I had similar code in one of my projects.

Both copies of consume have always had a path that modifies (*v)[0]. I believe it happens when there is a short write. Now (*v)[0] is modified in every case, making it clear that a copy is necessary.

the (Buffers).WriteTo method has always changed its receiver, my inclination is to document it.

🤔 I forget how I learned that it modified its receiver. I'm surprised that there is no mention of it on the type or method!

These perhaps?

@martin-sucha
Copy link
Contributor

Both copies of consume have always had a path that modifies (*v)[0]. I believe it happens when there is a short write.

I see, thanks for pointing this out! So we just need to add documentation for net.Buffers.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/387434 mentions this issue: net: document methods of Buffers

gopherbot pushed a commit that referenced this issue Feb 22, 2022
@martin-sucha @ianlancetaylor
net: document methods of Buffers

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
3517036 
There is code in the wild that copies the Buffers slice,
but not the contents.
Let's document explicitly that it is not safe to do so.

Updates #45163

Change-Id: Id45e27b93037d4e9f2bfde2558e7869983b60bcf
Reviewed-on: https://go-review.googlesource.com/c/go/+/387434
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
gopherbot pushed a commit that referenced this issue May 2, 2022
@oiooj @gopherbot
internal/poll: clear completed Buffers to permit earlier collection
32acceb 
Updates #45163

Change-Id: I73a6f22715550e0e8b83fbd3ebec72ef019f153f
Reviewed-on: https://go-review.googlesource.com/c/go/+/373374
Run-TryBot: Lee Baokun <bk@golangcn.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@golang golang locked and limited conversation to collaborators Feb 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@cbandy @jwebb @martin-sucha @ianlancetaylor @gopherbot and others