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

[Brian Burkhalter]

On Sat, 2 Dec 2023 03:48:46 GMT, jmehrens <duke at openjdk.org> wrote:

>> 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, pos, (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)];
>          }
> 
>             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?

I think that this is getting too complicated. For the time being, I think it would be better simply to have a conservative allow-list and trust only the classes in it. The approach can always be broadened at a later date, but at least for now there would be protection against untrustworthy `OutputStream`s

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16893#discussion_r1414382406