net: Buffers.WriteTo is prone to memory leaks

[jwebb]

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.

[gopherbot]

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

[cbandy]

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

• https://github.com/golang/go/blob/go1.17.3/src/net/writev_unix.go#L27

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]

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

[gopherbot]

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

[martin-sucha]

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]

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]

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]

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?

• net: add Buffers.ReadFrom and Buffers.Write? #17607
• net: make Buffers methods on value receivers #19680 (comment)
• net: Buffers makes multiple Write calls on Writers that don't implement buffersWriter #21676 (comment)

[martin-sucha]

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]

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