8220394: bufferedStream does not honor size limit

Thomas Stüfe thomas.stuefe at gmail.com
Wed May 22 07:00:48 UTC 2019


On Wed, May 22, 2019 at 8:53 AM David Holmes <david.holmes at oracle.com>
wrote:

> 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 ??
> }
>
>
:( you are right. I'm getting sloppy, too much patches in parallel. I will
post a new webrev shortly.

..Thomas


> 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
> >
> >
>


More information about the hotspot-runtime-dev mailing list