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

io: Copy implicilty requires WriterTo and ReaderFrom to not return io.EOF #44411

Open
cyphar opened this issue Feb 19, 2021 · 5 comments
Open
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@cyphar
Copy link

cyphar commented Feb 19, 2021

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

$ go version
go version go1.14.15 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cyphar/.cache/go-build"
GOENV="/home/cyphar/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/cyphar/.local"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib64/go/1.14"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib64/go/1.14/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build440126677=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/6dFx2mL7f5r

package main

import (
	"fmt"
	"io"
	"io/ioutil"
)

type dummyReader struct {}

func (r dummyReader) Read(p []byte) (n int, err error) {
	return 0, io.EOF
}

type dummyWriteTo struct {
	dummyReader
}

func (r dummyWriteTo) WriteTo(w io.Writer) (n int64, err error) {
	return 0, io.EOF
}

func main() {
	n, err := io.Copy(ioutil.Discard, dummyReader{})
	fmt.Printf("io.Copy using io.Reader -- n=%v, err=%v\n", n, err)
	
	n, err = io.Copy(ioutil.Discard, dummyWriteTo{})
	fmt.Printf("io.Copy using io.WriterTo -- n=%v, err=%v\n", n, err)
}

The primary issue is that if you have an io.Reader which implements WriteTo such that it returns io.EOF at the end of the stream, io.Copy will blindly forward that error. klauspost/pgzip#38 is an example of a fairly popular project which made this mistake, and I hit this in opencontainers/umoci#360.

The same thing happens if you return io.EOF from ReadFrom (which is a little bit weirder to do, but if you don't use io.Copy in the implementation of io.ReaderFrom you might do this by accident).

It seems this is either:

  • A documentation issue, since as far as I can tell, the io.WriterTo and io.ReaderFrom documentation doesn't mention that you shouldn't return io.EOF from those methods. The wording does kind of imply that you shouldn't return io.EOF but IMHO this should be much more explicit because any WriteTo implementation that doesn't use io.Copy internally will have to handle this explicitly and should be warned that this is the case.
  • A bug in io.Copy (and possibly related stdlib users of io.WriterTo and io.ReaderFrom), because io.EOF is handled when it is returned by Read.

What did you expect to see?

io.Copy using io.Reader -- n=0, err=<nil>
io.Copy using io.WriterTo -- n=0, err=<nil>

or documentation which mentions that you shouldn't return io.EOF from WriteTo (or ReadFrom).

What did you see instead?

io.Copy using io.Reader -- n=0, err=<nil>
io.Copy using io.WriterTo -- n=0, err=EOF
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 19, 2021
@ianlancetaylor
Copy link
Contributor

I think this is a documentation issue. I can't see any reason why WriteTo or ReadFrom should return io.EOF.

@ianlancetaylor ianlancetaylor added Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Feb 19, 2021
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Feb 19, 2021
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 19, 2021
dreamerjackson added a commit to dreamerjackson/go that referenced this issue Feb 21, 2021
@dreamerjackson
Copy link
Contributor

@ianlancetaylor i make a PR, can you help view it ? thanks!

@gopherbot
Copy link

Change https://golang.org/cl/294709 mentions this issue: fix: io/copy document. fix #44411

@cyphar
Copy link
Author

cyphar commented Feb 21, 2021

I can't see any reason why WriteTo or ReadFrom should return io.EOF.

You could do it by accident if you don't use io.Copy internally and instead use some other I/O methods, but if the semantics are basically "more efficient io.Copy" then I agree it should never return io.EOF. Thanks.

cyphar added a commit to cyphar/go that referenced this issue Feb 21, 2021
Because io.Copy delegates to WriteTo and ReadFrom if possible,
implementors need to be made aware that the EOF behaviour of WriteTo and
ReadFrom need to match the io.Copy semantics (do not return io.EOF --
even if no bytes are read).

In addition, the documentation of the two interfaces had diverged
slightly, so re-unify the wording.

Fixes: golang#44411
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
@gopherbot
Copy link

Change https://golang.org/cl/294769 mentions this issue: io: unify EOF documentation for ReadFrom and WriteTo

cyphar added a commit to cyphar/go that referenced this issue Feb 22, 2021
Because io.Copy delegates to WriteTo and ReadFrom if possible,
implementors need to be made aware that the EOF behaviour of WriteTo and
ReadFrom need to match the io.Copy semantics (do not return io.EOF --
even if no bytes are read).

In addition, the documentation of the two interfaces had diverged
slightly, so re-unify the wording.

Fixes golang#44411
YairHalberstadt added a commit to YairHalberstadt/ssm-session-client that referenced this issue Feb 8, 2023
1. WriteTo should not return EOF (see golang/go#44411).
2. In case of EOF, WriteTo should write the final payload before returning
3. In case of non EOF error, WriteTo should return n, not nw.
mmmorris1975 pushed a commit to mmmorris1975/ssm-session-client that referenced this issue Feb 10, 2023
1. WriteTo should not return EOF (see golang/go#44411).
2. In case of EOF, WriteTo should write the final payload before returning
3. In case of non EOF error, WriteTo should return n, not nw.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
5 participants