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

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

cyphar opened this issue Feb 19, 2021 · 5 comments

Comments

@cyphar
Copy link

@cyphar 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
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 19, 2021

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

@dreamerjackson
Copy link
Contributor

@dreamerjackson dreamerjackson commented Feb 21, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 21, 2021

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

@cyphar
Copy link
Author

@cyphar 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

@gopherbot gopherbot commented Feb 21, 2021

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants