RFR: 8266078: Reader.read(CharBuffer) advances Reader position for read-only Charbuffers
Please consider this request to modify `Reader.read(CharBuffer)` to check whether the buffer is read-only before reading any characters from the character stream. This can happen now if the buffer is read-only. Character are first read thereby advancing the stream before an attempt is made to put them in the `CharBuffer` thus incorrectly advancing the stream position. ------------- Commit messages: - 8266078: Reader.read(CharBuffer) advances Reader position for read-only Charbuffers Changes: https://git.openjdk.java.net/jdk/pull/3725/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3725&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266078 Stats: 70 lines in 2 files changed: 70 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3725.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3725/head:pull/3725 PR: https://git.openjdk.java.net/jdk/pull/3725
On Tue, 27 Apr 2021 18:47:41 GMT, Brian Burkhalter <bpb@openjdk.org> wrote:
Please consider this request to modify `Reader.read(CharBuffer)` to check whether the buffer is read-only before reading any characters from the character stream. This can happen now if the buffer is read-only. Character are first read thereby advancing the stream before an attempt is made to put them in the `CharBuffer` thus incorrectly advancing the stream position.
src/java.base/share/classes/java/io/Reader.java line 202:
200: if (target.isReadOnly()) 201: throw new ReadOnlyBufferException(); 202: int len = target.remaining();
It seems like the other branch of the if should also check for read-only and throw. Or can the target not have an array if it is readonly. It might return -1 for a readonly target if the source was used up. The always throw. Moving the `if (target.isReadOnly())` to the line 188: would be unambiguous. test/jdk/java/io/Reader/ReadIntoReadOnlyBuffer.java line 52:
50: try { 51: r.read(b); 52: throw new RuntimeException();
A helpful message would make it clearer what failed, when/if it fails. ------------- PR: https://git.openjdk.java.net/jdk/pull/3725
On Tue, 27 Apr 2021 19:31:40 GMT, Roger Riggs <rriggs@openjdk.org> wrote:
Please consider this request to modify `Reader.read(CharBuffer)` to check whether the buffer is read-only before reading any characters from the character stream. This can happen now if the buffer is read-only. Character are first read thereby advancing the stream before an attempt is made to put them in the `CharBuffer` thus incorrectly advancing the stream position.
src/java.base/share/classes/java/io/Reader.java line 202:
200: if (target.isReadOnly()) 201: throw new ReadOnlyBufferException(); 202: int len = target.remaining();
It seems like the other branch of the if should also check for read-only and throw. Or can the target not have an array if it is readonly. It might return -1 for a readonly target if the source was used up. The always throw.
Moving the `if (target.isReadOnly())` to the line 188: would be unambiguous.
It can't have an array if it is read-only. Returns: true if, and only if, this buffer is backed by an array and is not read-only https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/nio/CharBu...)
test/jdk/java/io/Reader/ReadIntoReadOnlyBuffer.java line 52:
50: try { 51: r.read(b); 52: throw new RuntimeException();
A helpful message would make it clearer what failed, when/if it fails.
Agreed. ------------- PR: https://git.openjdk.java.net/jdk/pull/3725
Please consider this request to modify `Reader.read(CharBuffer)` to check whether the buffer is read-only before reading any characters from the character stream. This can happen now if the buffer is read-only. Character are first read thereby advancing the stream before an attempt is made to put them in the `CharBuffer` thus incorrectly advancing the stream position.
Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8266078: Make read-only check global; add message in test ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/3725/files - new: https://git.openjdk.java.net/jdk/pull/3725/files/2fbaff7d..4a899b26 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3725&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3725&range=00-01 Stats: 8 lines in 2 files changed: 5 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3725.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3725/head:pull/3725 PR: https://git.openjdk.java.net/jdk/pull/3725
On Tue, 27 Apr 2021 20:25:32 GMT, Brian Burkhalter <bpb@openjdk.org> wrote:
Please consider this request to modify `Reader.read(CharBuffer)` to check whether the buffer is read-only before reading any characters from the character stream. This can happen now if the buffer is read-only. Character are first read thereby advancing the stream before an attempt is made to put them in the `CharBuffer` thus incorrectly advancing the stream position.
Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
8266078: Make read-only check global; add message in test
Marked as reviewed by rriggs (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/3725
On Tue, 27 Apr 2021 20:25:32 GMT, Brian Burkhalter <bpb@openjdk.org> wrote:
Please consider this request to modify `Reader.read(CharBuffer)` to check whether the buffer is read-only before reading any characters from the character stream. This can happen now if the buffer is read-only. Character are first read thereby advancing the stream before an attempt is made to put them in the `CharBuffer` thus incorrectly advancing the stream position.
Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
8266078: Make read-only check global; add message in test
Marked as reviewed by alanb (Reviewer). src/java.base/share/classes/java/io/Reader.java line 206:
204: char[] cbuf = new char[len]; 205: // If a read-only check had not been done above, then 206: // the stream would be incorrectly advanced here.
The RO check is good but I'm not sure that this comment is useful (it might confuse future maintainers), I would be tempted to leave that part out. ------------- PR: https://git.openjdk.java.net/jdk/pull/3725
Please consider this request to modify `Reader.read(CharBuffer)` to check whether the buffer is read-only before reading any characters from the character stream. This can happen now if the buffer is read-only. Character are first read thereby advancing the stream before an attempt is made to put them in the `CharBuffer` thus incorrectly advancing the stream position.
Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8266078: Remove confusing comment ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/3725/files - new: https://git.openjdk.java.net/jdk/pull/3725/files/4a899b26..8935981a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3725&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3725&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3725.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3725/head:pull/3725 PR: https://git.openjdk.java.net/jdk/pull/3725
On Wed, 28 Apr 2021 15:29:20 GMT, Brian Burkhalter <bpb@openjdk.org> wrote:
Please consider this request to modify `Reader.read(CharBuffer)` to check whether the buffer is read-only before reading any characters from the character stream. This can happen now if the buffer is read-only. Character are first read thereby advancing the stream before an attempt is made to put them in the `CharBuffer` thus incorrectly advancing the stream position.
Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
8266078: Remove confusing comment
Marked as reviewed by chegar (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/3725
On Tue, 27 Apr 2021 18:47:41 GMT, Brian Burkhalter <bpb@openjdk.org> wrote:
Please consider this request to modify `Reader.read(CharBuffer)` to check whether the buffer is read-only before reading any characters from the character stream. This can happen now if the buffer is read-only. Character are first read thereby advancing the stream before an attempt is made to put them in the `CharBuffer` thus incorrectly advancing the stream position.
This pull request has now been integrated. Changeset: 5f156660 Author: Brian Burkhalter <bpb@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/5f15666092d9928c07cbe66cdd96538459ff... Stats: 71 lines in 2 files changed: 71 ins; 0 del; 0 mod 8266078: Reader.read(CharBuffer) advances Reader position for read-only Charbuffers Reviewed-by: rriggs, alanb, chegar ------------- PR: https://git.openjdk.java.net/jdk/pull/3725
participants (4)
-
Alan Bateman
-
Brian Burkhalter
-
Chris Hegarty
-
Roger Riggs