RFR: 8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted [v2]

[jmehrens]

On Fri, 1 Dec 2023 23:21:35 GMT, Brian Burkhalter <bpb at openjdk.org> 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 at 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