Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted #16879

Closed
wants to merge 25 commits into from
+114 −3
Closed

8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted #16879

Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
6993265
8320971: Use BufferedInputStream.buf directly when param of implTrans…
stsypanov Nov 29, 2023
d5ab8bf
8320971: Use BufferedInputStream.class.getPackageName()
stsypanov Nov 29, 2023
7779aac
8320971: Trust any OutputStream from java.*
stsypanov Nov 30, 2023
a77208e
8320971: Add test
stsypanov Nov 30, 2023
b68eeba
Merge branch 'master' into 8320971
stsypanov Dec 8, 2023
69fb33f
8320971: Use same approach as BAOS
stsypanov Dec 8, 2023
9bf4e22
8320971: Rewrite comment
stsypanov Dec 8, 2023
6abc46d
8320971: Rearrange code
stsypanov Dec 8, 2023
f5cf134
8320971: Extract utility class
stsypanov Dec 10, 2023
8015687
8320971: Rename method
stsypanov Dec 11, 2023
2df52c7
Merge branch 'master' into 8320971
stsypanov Dec 11, 2023
3576a17
8320971: Move IOStreams to com.sun.io
stsypanov Dec 11, 2023
50e4e4e
8320971: Fix build
stsypanov Dec 11, 2023
a3ba43f
8320971: Whitespaces
stsypanov Dec 12, 2023
c6696e2
8320971: Fix JavaDoc
stsypanov Dec 13, 2023
42147ce
8320971: Add more tests
stsypanov Dec 13, 2023
8626e92
8320971: Revert irrelevant changes
stsypanov Dec 14, 2023
cbe7ac2
Merge branch 'master' into 8320971
stsypanov Dec 19, 2023
a5e05e8
8320971: Adjust JavaDoc
stsypanov Dec 19, 2023
84686bc
8322292: Remove TransferToTrusted.checkedOutputStream()
stsypanov Dec 19, 2023
261b473
8320971: Inline tests
stsypanov Dec 22, 2023
ee03599
Merge branch 'master' into 8320971
stsypanov Dec 22, 2023
41a7f1a
8320971: Fix test
stsypanov Dec 22, 2023
e769dfb
8320971: Fix JavaDoc
stsypanov Dec 30, 2023
8d15e74
Update TransferToTrusted.java
stsypanov Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/java.base/share/classes/java/io/BufferedInputStream.java
View file
Original file line number Diff line number Diff line change
@@ -643,8 +643,7 @@ private long implTransferTo(OutputStream out) throws IOException {
if (getClass() == BufferedInputStream.class && markpos == -1) {
int avail = count - pos;
if (avail > 0) {
// trust any OutputStream residing in java.*
if (out.getClass().getPackageName().startsWith("java.")) {
if (out.trusted()) {
out.write(getBufIfOpen(), pos, count);
} else {
// Prevent poisoning and leaking of buf
5 changes: 1 addition & 4 deletions src/java.base/share/classes/java/io/ByteArrayInputStream.java
View file
Original file line number Diff line number Diff line change
@@ -209,10 +209,7 @@ public synchronized long transferTo(OutputStream out) throws IOException {
if (len > 0) {
// 'tmpbuf' is null if and only if 'out' is trusted
byte[] tmpbuf;
Class<?> outClass = out.getClass();
if (outClass == ByteArrayOutputStream.class ||
outClass == FileOutputStream.class ||
outClass == PipedOutputStream.class)
if (out.trusted())
tmpbuf = null;
else
tmpbuf = new byte[Integer.min(len, MAX_TRANSFER_SIZE)];
13 changes: 12 additions & 1 deletion src/java.base/share/classes/java/io/OutputStream.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1994, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1994, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -204,4 +204,15 @@ public void flush() throws IOException {
public void close() throws IOException {
}

/**
* Checks whether this OutputStream is trusted, i.e. doesn't modify written byte[]
*
* @return true if the argument of {@link #write(byte[])}} and {@link #write(byte[], int, int)}} needn't be copied
*/
boolean trusted() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a strange construction. Any subclass could simply implement this as return true;. Where is the guard against this, and why not doing it that way?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically speaking, OutputStream is an abstract class, so this declaration of boolean trusted() is a package-protected method that will be visible and overridable only within JDK itself.
However, I agree it looks suspicious.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on this method doesn't look right. The issue for BAIS is that its lifetime may be different to the byte[] that it wraps, think about use after the BAIS has been discarded. You don't want a sink keeping a reference to the byte[] even if it doesn't scribble on it.

Copy link
Contributor Author

@stsypanov stsypanov Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkarg I guess the method can only be implemented by subclasses residing in the same package with OutputStream, right?

@AlanBateman fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stsypanov Yes but still it is just weird to ask any output stream if it is trusted. I mean, it feels just unsecure to ask: "Do you pretend to be trusted?" instead of "Do we trust you?". I could sleep better if this method would not be part of each OutputStream subclass. We should either move it back to the place where needed, or into some static utility like OutputStreams::isTrusted(OutputStream) (mind the plural!), or it should at least be final.

Copy link
Contributor

@ecki ecki Dec 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Deleted) the new version doesn’t have the issue (albeit now it’s rather complicated formulated) “If stream can be used by JCL callers without extra copies of the byte[] argument to write(). This is not over writeable by user code.”

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike BAIS, the BufferedInputStream can wrap an untrusted InputStream. BIS.buf is passed directly to wrapped InputStream so I would assume that we would want to avoid exposing BIS.buf to the out parameter of transferTo. This way we know the input stream is not able to poison the output stream when a write is in progress.

I assume that the current small list of trusted classes are also immune to poison so I imagine this patch is safe. However, for any FilterInputStream we should either always use Arrays.copyOfRange because the input side can poison the output side or it needs a mirroring allow list for the target input stream to insure that the wrapped input stream is not poisoning the out parameter.

For instance, java.util.zip.CheckedOutputStream in theory could be added as trusted class as it doesn't leak or poison but, looking at the code it would appear that it is not immune to poison.

var clazz = getClass();
return clazz == ByteArrayOutputStream.class
|| clazz == FileOutputStream.class
|| clazz == PipedOutputStream.class;
}
}