If I'm relying on io.LimitedReader to protect a region of memory, then it can be a critical issue when used with io.TeeReader

io.TeeReader(io.LimitReader(n), buf)) // constraint: no access to buf beyond n

In #22097, I mentioned that this is a slippery slope and I'm hesitant to add further checks unless we consciously make a decision that we want all functions that take in an io.Reader to protect against invalid readers (and writers).

Adding the checks to bufio.Reader and bytes.Buffer.ReadFrom was fine since those are probably the most common vectors through which a bad reader would be detected.

There are several broader question at hand:

• Should users of io.Reader and io.Writer be responsible for checking non-compliant implementations?
  • If so, what type of non-compliant issues do we check for? Negative counts are hardly the only issue.
    • Do we check that Read returns a count within 0 <= n <= len(b)?
    • Do we check that Write returns a count within 0 <= n <= len(b)?
    • Do we check that Write returns n == len(b) if err == nil?
  • What do you do when you discover an error? Do we panic (that's what bufio.Reader does). Do we convert it into an error? Do you treat a negative count as equivalent to zero?
• Where should we perform these checks?
  • I can see at least the following needing to be addressed: io.Copy, io.CopyBuffer, io.CopyN, io.ReadAtLeast, io.ReadFull, io.LimitedReader, io.MultiReader, io.TeeReader, io.SectionReader, ioutil.ReadAll, etc...

Given the large scope creep that adding these checks will cause, I'm hesitant to penalize all users of io.Reader to handle invalid implementations.

I don't even know what LimitReader means for a broken Reader. I don't see why any particular behavior is correct.

If your code base is encountering a number of invalid implementations of io.Reader, I would suggest wrapping your readers in CheckReader and see what types of things fail.

Even better, for all the implementations of io.Reader, do something similar in your unit tests.

I'm going to decline this issue. We are not going to add checks in the standard library for all possible erroneous behavior by io.Reader values.

@dsnet

1. Your concerns are valid. On the one hand I agree that is a slippery slope, but I don't like that clients should totally trust the io.Reader contract only. On the other hand that is a protocol that have been agreed by both sides for communication.

2. I like the idea with CheckReader/CheckWriter, and I would add a check to consecutive empty reads with 0, nil ( e.g. 100 consecutive empty reads, as bufio.maxConsecutiveEmptyReads )

What do you think, should we add those checkers to io package?
I believe that it might be a good way of checking an io.Reader implementation ( the only source of the truth; automatic self-test in units ioutil.ReadAll(io.CheckReader(CustomReader))) )

Those types should not be added to io since they are primarily for testing and their utility at large is pretty low. It could possibly be added to the testing/iotest package, but I personally find that package awkward and is often more problematic than helpful.

Given the simplicity of the code, it's not bad maintaining it yourself. I don't see a strong need for it to be in the standard library.

@dsnet thank you for explanation