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