8220394: bufferedStream does not honor size limit

[David Holmes]

Hi Thomas,

Sorry but I don't grok this stop-gap fix. I get that bufferedStream is a 
strange entity - and to me it doesn't look or act like a buffered 
stream! - but other code seems to recognise this e.g. see:

./share/code/codeHeapState.cpp

and look what it does to effect actual buffering between a 
bufferedStream and a real outputStream!

If there is code that misuses bufferedStream (and it seems the only 
other direct uses are in the attach listener and JFR) then I'd rather 
see those fixed. But looking at the attach listener code it also seems 
to recognise that bufferedStream is just a "buffer" which it copies to 
the final destination ie. writing the content to the socket.

So where is the code that misuses bufferedStream?

Thanks,
David
-----

On 21/05/2019 4:11 pm, Thomas Stüfe wrote:
> Ping?
> 
> 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
>>
>>
>>