8220394: bufferedStream does not honor size limit

[Thomas Stüfe]

Hi all,

third round:
http://cr.openjdk.java.net/~stuefe/webrevs/8220394-bufferedstream-does-not-honor-size-limit/webrev.02/webrev/

I also added a gtest test case for bufferedStream. It tests the static
variant, the dynamic variant without truncation and with truncation. The
last test, testing with truncation, is commented out - to run this test,
you need to manually enable it first. This is because it uses a lot of
memory (100M) in release and in debug it would crash as expected.

Cheers, Thomas




On Tue, May 21, 2019 at 4:18 PM Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

> Hi,
>
> new webrev with the following changes:
>
> - reworked comment to make it more concise and clear
> - the cap where we assert now is guaranteed to be larger than the buffer
> maximum.
>
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8220394-bufferedstream-does-not-honor-size-limit/webrev.01/webrev/
>
> Thanks, Thomas
>
>
>
> On Tue, May 14, 2019 at 12:25 PM Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
>
>>
>>    Hi all,
>>
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8220394
>> cr:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8220394-bufferedstream-does-not-honor-size-limit/webrev.00/webrev/
>>
>> I redid the patch for this in a minimal form. The first version I posted
>> in March but after consideration I found it too invasive.
>>
>> Short story: bufferedStream is misused as a
>> outputStream-with-dynamic-buffer in a number of places - misused since we
>> have stringStream for that.
>>
>> It has something looking like a maximum buffer size cap but that is
>> actually a flush-trigger for child classes of bufferesStream.
>> bufferedStream::flush itself is a noop.
>>
>> That means that printing to this stream may cause high memory footprint
>> or native OOM since the upper limit is not honored. In runaway printing
>> coding this can tear down the process.
>>
>> This patch is a stopgap - when we reach the buffer limit - but not below
>> 100M - we will assert(debug) or truncate (release).
>>
>> I am careful here since I do not know if there are situations where more
>> than buffer-limit bytes are written today and suddenly enforcing this limit
>> now would cause errors. I think 100M is safe to be considered "too much".
>>
>> The real correct solution should be that all callers use stringStream and
>> handle truncation. This patch is small and could be, if necessary, ported
>> down to older releases easily.
>>
>> Thanks, Thomas
>>
>>
>>