-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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/ioutil: Discard's ReadFrom method's intentional raciness is actually harmful #4589
Labels
Milestone
Comments
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!" :) |
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 } |
This issue was closed by revision eb43ce2. Status changed to Fixed. |
pwaller
added a commit
to pwaller/git
that referenced
this issue
May 24, 2015
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.
pwaller
added a commit
to pwaller/git
that referenced
this issue
May 24, 2015
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.
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
The text was updated successfully, but these errors were encountered: