RFR: 8294696 - BufferedInputStream.transferTo should drain buffer when mark set

[Alan Bateman]

On Mon, 3 Oct 2022 05:50:50 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> I think the leak is actually not a problem. `ByteArrayInputStream::transferTo` does the same BTW, see https://github.com/openjdk/jdk/blob/68da02c7b536799ccca49e889c22f3e9a2691fb7/src/java.base/share/classes/java/io/ByteArrayInputStream.java#L206-L211
>> In case the wrapped stream would write into the leaked buffer, that "injected" data would be lost, as no replay can happen due to the `mark == -1`  safety check. So unless we remove that check (which is not planned), such "injected" data is never read.
>> 
>> Regarding the `-1` check: This is fixed in 0aee81f1f3269fa05b9c04ea42343953ad758100. I proposed that change to align it with *all other* checks of `markpos` in the existing source code of BIS. I can undo that change, but following your warning, shouldn't we fix *all other* checks (independent of this PR) from `markpos < 0` to `markpos == -1` then?
>
>> I think the leak is actually not a problem. 
> 
> BAIS is a fixed size. This is different to BIS where it wraps an input stream that may be changing, e.g. create an input stream to a file that is growing and call transferTo several times then you'll see what I mean. We need to mark sure we look at this very closely, that is all I'm saying.
> 
>> Regarding the `-1` check: I did that to align it with _all other_ checks of `markpos` in the existing source code of BIS. I can undo that change, but following your warning, shouldn't we fix _all other_ checks (independent of this PR) from `markpos < 0` to `markpos == -1` then?
> 
> Probably yes.

> @AlanBateman WDYT? Is such a test mandatory to include this rather simple PR?

I think it means checking that test/jdk/java/io/BufferedInputStream/TransferTo.java exercises this code path, I expect it should.

It might seem a "rather simple PR" but it leaks a reference to the internal buffer to the target and we need to get comfortable with that. It only does then when there is no mark (good) so this should mostly limit the concern to sources where EOF may be encountered more than once.

-------------

PR: https://git.openjdk.org/jdk/pull/10525