io/ioutil: Discard's ReadFrom method's intentional raciness is actually harmful #4589

Closed
bradfitz opened this Issue Dec 26, 2012 · 4 comments

Comments

Projects
None yet
4 participants
Owner

bradfitz commented Dec 26, 2012

After a couple days of on & off frustrated debugging, I've just discovered a bug in
ioutil.Discard.

The original version of ioutil.Discard was safe:
https://code.google.com/p/go/source/detail?r=cb593b108aa5 , it only had  Write method.

A later commit added a ReadFrom method:
https://code.google.com/p/go/source/detail?r=13faa632ba3a#

That ReadFrom CL uses an intentionally-racy global variable, blackHole, which is re-used
for all Read calls.

io.Copy uses ReadFrom, if available:

    func Copy(dst Writer, src Reader) (written int64, err error) {
            // If the writer has a ReadFrom method, use it to do the copy.                                                                                                                                                              
            // Avoids an allocation and a copy.                                                                                                                                                                                         
            if rt, ok := dst.(ReaderFrom); ok {
                   return rt.ReadFrom(src)
            }
            ......

The race detector had a problem with this,
https://golang.org/issue/3970 , and so worked around it by using a
safe version: https://code.google.com/p/go/source/detail?r=1e55cf10aa4f#

And I just hit this problem in a real program, getting corrupt SHA-1 results from files.

In my program, I had a io.Reader wrapper which calculated the SHA-1 of contents as they
were read.

In some cases, my code did:

     io.Copy(ioutil.Discard, sha1TrackingReader)

... which resulted in calls to ioutil.Discard.ReadFrom(sha1TrackingReader) and meant
that my sha1TrackingReader got the same racy buffer provided to it from multiple
concurrent goroutines.

Hence my SHA-1 corruption.

My workaround fix
(camlistore/camlistore@22621b8
was:

      _, err = io.Copy(struct{ io.Writer }{ioutil.Discard}, td)

Considering how much time it took me to debug this, how surprising it was, and how it
broke composition of standard interface types, I think this should be fixed.  If we do a
Go1.0.4, I'd like it to be fixed there, too.

The memory allocation reduction (of reusing the global variable) can be obtained with a
simple byte pool channel instead.
Owner

bradfitz commented Dec 26, 2012

Member

dvyukov commented Dec 27, 2012

Comment 3:

Oops, I did not foresee such possibility.
What exactly has happened? The SHA1 reader is expecting that the buffer passed to Read()
is not shared, and so the reader is writing to it and then re-reading from it to
calculate hash, right?
I think the SHA1 reader behavior is completely reasonable and legal.
Now I need to make an old age voice and say "Didn't I tell you about it!" :)
Owner

bradfitz commented Dec 27, 2012

Comment 4:

Yes, the SHA1 reader assumed it owned the buffer and could use it to call sha1
Write([]byte):
type trackDigestReader struct {
        r io.Reader
        h hash.Hash
}
func (t *trackDigestReader) Read(p []byte) (n int, err error) {
        if t.h == nil {
                t.h = sha1.New()
        }
        n, err = t.r.Read(p)    // data race!
        t.h.Write(p[:n])      // data race!
        return
}
Owner

bradfitz commented Dec 28, 2012

Comment 5:

This issue was closed by revision eb43ce2.

Status changed to Fixed.

bradfitz self-assigned this Dec 28, 2012

rsc added this to the Go1.1 milestone Apr 14, 2015

rsc removed the go1.1 label Apr 14, 2015

@pwaller pwaller added a commit to pwaller/git that referenced this issue May 24, 2015

@pwaller pwaller Check error returns of Seek, fix global data race
This code had a race in it, which actually Go itself once had
in ioutil.Discard but subsequently got fixed:

golang/go#4589

In addition, error checking for Seek has been added.

I've taken the liberty of removing some convenience definitions
because some of them just aliased standard library functions
making it harder to understand the code. This also shortens the
code.
8978f79

@pwaller pwaller added a commit to pwaller/git that referenced this issue May 24, 2015

@pwaller pwaller Check error returns of Seek, fix global data race
This code had a race in it, which actually Go itself once had
in ioutil.Discard but subsequently got fixed:

golang/go#4589

So `readerSkip` has been replaced with a copy to `ioutil.Discard`.

In addition, error checking for `Seek` has been added.

I've taken the liberty of removing some convenience definitions
because some of them just aliased standard library functions
making it harder to understand the code and spot that there were
unchecked errors. This also shortens the code.
564c5fe

gopherbot locked and limited conversation to collaborators Jun 24, 2016

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.