On Mon, 3 Oct 2022 05:50:50 GMT, Alan Bateman <al...@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
