On Wed, 29 Nov 2023 22:55:41 GMT, Brian Burkhalter wrote:
> > As Alan pointed out, it is a bug (actually even a security risk), so BAIS
> > should get fixed, too.
>
> I am going to file an issue on this.
Thank you, I just planned to fix this today when I saw your existing PR! 👍
-
On Wed, 29 Nov 2023 22:49:58 GMT, Markus KARG wrote:
> As Alan pointed out, it is a bug (actually even a security risk), so BAIS
> should get fixed, too.
I am going to file an issue on this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/10525#discussion_r1409965074
On Wed, 29 Nov 2023 05:55:03 GMT, Vladimir Sitnikov
wrote:
>> src/java.base/share/classes/java/io/BufferedInputStream.java line 612:
>>
>>> 610: if (avail > 0) {
>>> 611: // Prevent poisoning and leaking of buf
>>> 612: byte[] buffer = Arrays.copyOfRa
On Wed, 29 Nov 2023 22:49:17 GMT, Markus KARG wrote:
>> Buffer copy was not there before, and defensive copy was never present in
>> `ByteArrayInputStream` as well:
>> https://github.com/openjdk/jdk/blob/9a6ca233c7e91ffa2ce9451568b3be88ccd04504/src/java.base/share/classes/java/io/ByteArrayInput
On Wed, 29 Nov 2023 05:48:58 GMT, Vladimir Sitnikov
wrote:
>> Markus KARG has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Alternative C + Arrays.copyOfRange()
>
> src/java.base/share/classes/java/io/BufferedInputStream.java line 612:
>
On Mon, 31 Oct 2022 13:30:13 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Alternative C + Arrays.copyOfRange()
src/java.base/share/classes/java/io/BufferedInputS
On Mon, 31 Oct 2022 13:30:13 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Alternative C + Arrays.copyOfRange()
Marked as reviewed by alanb (Reviewer).
-
On Mon, 31 Oct 2022 13:30:13 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Alternative C + Arrays.copyOfRange()
Looks all right.
-
Marked as reviewe
On Mon, 31 Oct 2022 09:52:06 GMT, Alan Bateman wrote:
>> **TL;DR:** Your optimization is now contained in this PR and outperforms the
>> previous code. Nevertheless I plead for Alternative C (double buffering)
>> instead, which is even faster but much simpler.
>>
>> **Justification**
>>
>> I
On Mon, 31 Oct 2022 09:52:06 GMT, Alan Bateman wrote:
>> **TL;DR:** Your optimization is now contained in this PR and outperforms the
>> previous code. Nevertheless I plead for Alternative C (double buffering)
>> instead, which is even faster but much simpler.
>>
>> **Justification**
>>
>> I
> This PR implements JDK-8294696.
Markus KARG has updated the pull request incrementally with one additional
commit since the last revision:
Alternative C + Arrays.copyOfRange()
-
Changes:
- all: https://git.openjdk.org/jdk/pull/10525/files
- new: https://git.openjdk.org/jdk/
On Mon, 31 Oct 2022 09:52:06 GMT, Alan Bateman wrote:
> > **TL;DR:** Your optimization is now contained in this PR and outperforms
> > the previous code. Nevertheless I plead for Alternative C (double
> > buffering) instead, which is even faster but much simpler.
> You had dismissed this previ
On Mon, 31 Oct 2022 08:28:07 GMT, Markus KARG wrote:
> **TL;DR:** Your optimization is now contained in this PR and outperforms the
> previous code. Nevertheless I plead for Alternative C (double buffering)
> instead, which is even faster but much simpler.
You had dismissed this previously so
On Sat, 29 Oct 2022 12:31:41 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> No immediate buffer resize; in the rare case of read-past-EOF automatic
> buffer grow w
> This PR implements JDK-8294696.
Markus KARG has updated the pull request incrementally with one additional
commit since the last revision:
No immediate buffer resize; in the rare case of read-past-EOF automatic
buffer grow will happen anyways
-
Changes:
- all: https://git.op
On Sun, 23 Oct 2022 08:40:35 GMT, Markus KARG wrote:
> Silly me, you are certainly right! I have modified the code as you proposed,
> so now we should be safe, finally. :-)
The updated version (7350a533) looks safe but it makes for a more complicated
transferTo so it would be good to see updat
On Sun, 23 Oct 2022 06:21:02 GMT, Alan Bateman wrote:
> It helps a bit but it still leaks the bytes in 0 to pos, and count
> buf.length. I think we have to assume that Dr. Evil's output stream will
> throw an exception so the code to replace the buffer won't run. This means
> replaces buf befo
> This PR implements JDK-8294696.
Markus KARG has updated the pull request incrementally with one additional
commit since the last revision:
Prevent leaking and poisoning
-
Changes:
- all: https://git.openjdk.org/jdk/pull/10525/files
- new: https://git.openjdk.org/jdk/pull/10
On Wed, 19 Oct 2022 08:31:07 GMT, Alan Bateman wrote:
>> Markus KARG has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Checking explicitly -1 instead of < 0
>
>> * Alternative B: Instead of double-buffering I drop the original buffer and
On Wed, 19 Oct 2022 08:31:07 GMT, Alan Bateman wrote:
>> Markus KARG has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Checking explicitly -1 instead of < 0
>
>> * Alternative B: Instead of double-buffering I drop the original buffer and
> This PR implements JDK-8294696.
Markus KARG has updated the pull request with a new target base due to a merge
or a rebase. The incremental webrev excludes the unrelated changes brought in
by the merge/rebase. The pull request contains three additional commits since
the last revision:
- Alt
On Wed, 19 Oct 2022 08:31:07 GMT, Alan Bateman wrote:
> > * Alternative B: Instead of double-buffering I drop the original buffer and
> > use a same-size replacement each time the buffer was drained inside of
> > `transferTo`. This is a rather cheap solution and rather perfectly prevents
> > O
On Mon, 3 Oct 2022 05:54:31 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Checking explicitly -1 instead of < 0
> * Alternative B: Instead of double-buffering I d
On Mon, 3 Oct 2022 05:54:31 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Checking explicitly -1 instead of < 0
I think before we give up, we should consider the
On Mon, 3 Oct 2022 05:54:31 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Checking explicitly -1 instead of < 0
> Thank you. So far I have not measured the actual
On Tue, 18 Oct 2022 17:02:31 GMT, Alan Bateman wrote:
> I sent a link to this PR to one of the security engineers and they share the
> concern. Have you done any performance testing with an implementation that
> makes a defensive copy?
Thank you. So far I have not measured the actual performan
On Tue, 18 Oct 2022 08:19:41 GMT, Markus KARG wrote:
> Does "security review" mean, that I shall proof the absence of the problem,
> or does that term mean a formal process in the OpenJDK organization (and how
> do I trigger it)?
I sent a link to this PR to one of the security engineers and th
On Tue, 18 Oct 2022 08:17:55 GMT, Alan Bateman wrote:
> The test is not the issue, it's that the proposal will require a security
> review. Yes, copying the bytes would remove that concern.
Does "security review" mean, that I shall proof the absence of the problem, or
does that term mean a for
On Mon, 3 Oct 2022 05:54:31 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Checking explicitly -1 instead of < 0
The test is not the issue, it's that the proposal
On Mon, 17 Oct 2022 11:14:35 GMT, Alan Bateman wrote:
>>> > @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.
>>>
On Mon, 17 Oct 2022 10:59:21 GMT, Roman Kennke wrote:
> Which kind of sources would do that, and is there a way to check it and
> prevent it?
>
> The whole issue points to an insufficiency in the API: it is lacking a way to
> transfer buffers in a read-only fashion (like NIO could do). I don't
On Sat, 15 Oct 2022 14:38:04 GMT, Alan Bateman wrote:
> > @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 s
On Sat, 15 Oct 2022 13:17:27 GMT, Markus KARG wrote:
> Isn't a test needed here which fails without but passes with the proposed
> change?
If you are saying, that a test should verify that, e.g. the wrapped OS receives
the buffer upon calling BIS.transferTo() under certain circumstances, then
On Mon, 3 Oct 2022 05:50:50 GMT, Alan Bateman 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/ByteArrayInputStre
On Mon, 3 Oct 2022 05:54:31 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Checking explicitly -1 instead of < 0
So how to go on?
-
PR: https://git.o
On Mon, 3 Oct 2022 05:54:31 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Checking explicitly -1 instead of < 0
> > Isn't a test needed here which fails without b
On Mon, 10 Oct 2022 20:41:39 GMT, Brian Burkhalter wrote:
> Isn't a test needed here which fails without but passes with the proposed
> change?
I do not see such a *need*, as this is solely a performance improvement, not a
fixed bug or added functionality. If the majority of this team wants to
On Mon, 3 Oct 2022 05:54:31 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Checking explicitly -1 instead of < 0
Isn't a test needed here which fails without but p
On Mon, 3 Oct 2022 05:54:31 GMT, Markus KARG wrote:
>> This PR implements JDK-8294696.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Checking explicitly -1 instead of < 0
@bplb Kindly requesting review. 😃
-
On Mon, 3 Oct 2022 05:50:50 GMT, Alan Bateman wrote:
> > 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
> >
On Sun, 2 Oct 2022 17:57:35 GMT, Markus KARG wrote:
> 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. write an input stream to a file that is growing and
call transferTo several times then you
> This PR implements JDK-8294696.
Markus KARG has updated the pull request incrementally with one additional
commit since the last revision:
Checking explicitly -1 instead of < 0
-
Changes:
- all: https://git.openjdk.org/jdk/pull/10525/files
- new: https://git.openjdk.org/jdk
42 matches
Mail list logo
