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

Jump to bottom

io: regression in MultiReader.Read causes nil panic #18232

Closed
dsnet opened this issue Dec 7, 2016 · 5 comments
Closed

io: regression in MultiReader.Read causes nil panic #18232

dsnet opened this issue Dec 7, 2016 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8

Comments

@dsnet
Copy link
Member

dsnet commented Dec 7, 2016 •
edited

The combination of the following CLs causing a nil panic:

  • CL/17873: make chained multiReader Read more efficient
  • CL/28533: make MultiReader nil exhausted Readers for earlier GC

The first CL allows MultiReader to "inherit" another MultiReader's list of io.Reader. This is problematic when combined with the later CL since that can set readers in that list to nil.

Reproduction: https://play.golang.org/p/jPl_pRr3ul

mr1 := io.MultiReader(strings.NewReader("def"), strings.NewReader("ghi"))
mr2 := io.MultiReader(strings.NewReader("abc"), mr1)

io.ReadFull(mr2, make([]byte, 4))
io.ReadFull(mr1, make([]byte, 4))
io.ReadFull(mr2, make([]byte, 3))

Discussion: https://groups.google.com/forum/#!topic/golang-dev/a5K3132l3_E

\cc @rsc @bradfitz @ncw
@dsnet dsnet added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 7, 2016
@dsnet dsnet added this to the Go1.8 milestone Dec 7, 2016
@rsc
Copy link
Contributor

rsc commented Dec 7, 2016

Suggested fix is to change= nil to = eofReader where eofReader is some pre-allocated reader that does the obvious thing. That will make sure the behavior of io.MultiReader(nil) does not change.

@dsnet
Copy link
Member Author

dsnet commented Dec 7, 2016

@rsc, would you like me to make the change?

@bradfitz
Copy link
Contributor

bradfitz commented Dec 7, 2016

Go for it. Otherwise I can if you're busy.

@dsnet dsnet self-assigned this Dec 7, 2016
@gopherbot
Copy link

CL https://golang.org/cl/34140 mentions this issue.

@ncw
Copy link
Contributor

ncw commented Dec 9, 2016

I can confirm that this fixes my original issue - thanks for tracking it down and making a fix :-)

@golang golang locked and limited conversation to collaborators Dec 9, 2017
@rsc rsc unassigned dsnet Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
5 participants
@bradfitz @rsc @ncw @dsnet @gopherbot