On Fri, 1 Dec 2023 23:21:35 GMT, Brian Burkhalter <[email protected]> wrote:
>> The case of `Channels.newOutputStream(AsynchronousByteChannel)` could be
>> handled by changing the return value of that method. For example,
>> `sun.nio.ch.Streams` could have a method `OutputStream
>> of(AsynchronousByteChannel)` added to it which returned something like an
>> `AsynChannelOutputStream` and we could use that.
>>
>> That said, it is true that a deny list is not inherently future-proof like
>> an allow list, as stated.
>
> I think that a sufficiently future-proof deny list could be had by changing
>
> 211 if (out.getClass().getPackageName().startsWith("java.") &&
>
> back to
>
> 211 if ("java.io".equals(out.getClass().getPackageName()) &&
>
> That would for example dispense with the problematic
> `Channels.newOutputStream(AynsynchronousByteChannel)` case:
>
> jshell> AsynchronousSocketChannel asc = AsynchronousSocketChannel.open()
> asc ==> sun.nio.ch.UnixAsynchronousSocketChannelImpl[unconnected]
>
> jshell> OutputStream out = Channels.newOutputStream(asc)
> out ==> java.nio.channels.Channels$2@58c1670b
>
> jshell> Class outClass = out.getClass()
> outClass ==> class java.nio.channels.Channels$2
>
> jshell> outClass.getPackageName()
> $5 ==> "java.nio.channels"
Even if scope is limited to `java.io` you have deal with FilterOutputStream and
ObjectOutputStream. I still haven't done a complete search so there could be
other adapters I've yet to review.
Thinking of a different approach, what if ByteArrayInputStream actually
recorded and used `readlimit` of the `mark` method? This allows us to safely
leak or poison 'this.data' because once transferTo is called we safely change
owner of the byte array if we know this stream is allowed to forget it existed.
Effectively you could do optimizations like this (didn't test or compile this):
public synchronized long transferTo(OutputStream out) throws IOException {
int len = count - pos;
if (len > 0) {
byte[] data = this.data;
byte[] tmp = null;
if (this.readLimit == 0) { //<- recorded by mark method, initial value
on construction of this would be zero.
data = this.data; //swap owner of bytes
this.data = new byte[0];
Arrays.fill(data, 0, mark, (byte) 0); // hide out of bounds data.
Arrays.fill(data, count, data.length, (byte) 0);
} else {
tmp = new byte[Integer.min(len, MAX_TRANSFER_SIZE)];
}
//original code could be optimized more given above.
while (nwritten < len) {
int nbyte = Integer.min(len - nwritten, MAX_TRANSFER_SIZE);
out.write(buf, pos, nbyte);
if (tmp != null) {
System.arraycopy(buf, pos, tmp, 0, nbyte);
out.write(tmp, 0, nbyte);
} else
out.write(buf, pos, nbyte);
pos += nbyte;
nwritten += nbyte;
}
assert pos == count;
if (data.length ==0) { //uphold rules of class.
pos = count = mark = 0;
}
}
return len;
}
This would approach avoids having to maintain an allow or deny list. The
downside of this approach and that is the constructor of ByteInputStream
doesn't copy the byte[] parameter. The caller is warned about this in the
JavaDocs but it might be shocking to have data escape ByteArrayInputStream.
Maybe that is deal breaker? Obviously there a compatibility issue with
recording readLimit in the mark method as it states it does nothing.
Thoughts?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16893#discussion_r1412715845
