proposal: io: allocate copy buffers from a pool

[neild]

https://go.dev/cl/456555 changed io.Copy to allocate its copy buffers from a sync.Pool. This exposed a race condition in the HTTP/2 ResponseWriter #58446, which can be fixed, but I'm taking this as a sign that any change should be made more judiciously; https://go.dev/cl/467095 will revert that change.

io.Copy, prior to CL 456555, allocates a 32k []byte buffer (or a smaller one if the source is an *io.LimitedReader with a size less than 32k). Allocating these buffers from a pool reduces GC allocations and notably improves io.Copy performance; see benchmarks in that CL's description. The net/http package maintains its own pool of copy buffers for this reason. Having every package that uses io.Copy maintain its own buffer pool seems unfortunate.

If the destination of a Copy retains the buffer passed to the Write call after returning, this results in a race condition where the buffer may be recycled through the pool while the Write is still in progress. This behavior is a violation of the io.Writer contract, but as #58446 demonstrates, this does happen in practice.

Perhaps we should start by detecting io.Writers that retain the buffer. When the race detector is enabled, io.Copy could write over the copy buffer after each call to Write. This will help the race detector to identify cases where another goroutine continues to access the buffer, as well as invalidating the buffer contents. https://go.dev/cl/466865 contains this change.

[dsnet]

The fmt package uses a sync.Pool for buffers, and passes that to the user-provided io.Writer in fmt.FprintXXX. Also, the json package pools the encoder state which includes a buffer that is also eventually passed to a user-provided io.Writer. Thus, there's already existing precedence of stdlib for pooling buffers that escape through io.Writer.Write.

[mvdan]

Using the race detector sounds like a good idea. I wonder if we could use it more broadly for Write calls: the compiler could instrument call sites to detect bad implementations, such as:

• make a copy of the buffer
• call Write with the copied buffer
• fill the copied buffer to prevent further use

It would add some overhead for sure, but it would cover writes happening outside io.Copy.

[gopherbot]

Change https://go.dev/cl/466865 mentions this issue: io: detect Writers that access io.Copy's buffer after returning

[bcmills]

I wonder if we could use [the race detector] more broadly for Write calls

I don't think we should do that as a compiler transformation, because in general a Go program might have side-channel information about an implementation and the compiler's job is only to implement the language spec.

That said, it does seem easy to implement an io.Writer wrapper that uses a buffer to check for violations (even on non-race enabled builds): https://go.dev/play/p/FskHhByVbhN

We could use that implementation under the race detector in libraries like io and fmt, and also offer it as a library (maybe in the io package?) for external use. 🤔

[mvdan]

I like that idea. Reminds me of https://pkg.go.dev/testing/iotest.

This pattern could be generalised to other similar interfaces, with parameters documented as "must not be retained". That's why my mind was going for the compiler - if it knew about those one way or another, it could enable the extra checks on -race without the developer needing to add extra code in.

[nedataghizadeh79]

The change introduced a buffer allocation optimization using a sync.Pool, but this resulted in a race condition in the HTTP/2 ResponseWriter. The suggested fix involves detecting io.Writers that retain the buffer and invalidating buffer contents after each Write call.

[AlexanderYastrebov]

That said, it does seem easy to implement an io.Writer wrapper that uses a buffer to check for violations (even on non-race enabled builds): https://go.dev/play/p/FskHhByVbhN

Shouldn't it check the hash after write call is complete? Or do I miss the idea?
Here is an example of corrupting writer that it does not detect https://go.dev/play/p/q8Ya6AMWhKX