8220394: bufferedStream does not honor size limit

[David Holmes]

Hi Thomas,

On 22/05/2019 12:18 am, Thomas Stüfe 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/

AFAICS this doesn't actually silently truncate but just drops a 
too-large message. If the next message is smaller it will get logged and 
you won't know something larger got dropped. Shouldn't it actually fill 
the remaining space with as much of the message as possible without 
allowing it to grow past the cap? Possibly by just adjusting len down:

if (buffer_pos + len > reasonable_cap) {
    assert(false, "Exceeded max buffer size for this string.");
    // truncate
    len = reasonable_cap - buffer_pos; // +/-1 maybe ??
}

Thanks,
David

> Thanks, Thomas
> 
> 
> 
> On Tue, May 14, 2019 at 12:25 PM Thomas Stüfe <thomas.stuefe at gmail.com 
> <mailto: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
> 
>