8220394: bufferedStream does not honor size limit

[Thomas Stüfe]

Thanks a lot, Christoph!

On Thu, May 23, 2019, 23:01 Langer, Christoph <christoph.langer at sap.com>
wrote:

> Hi Thomas,
>
> this change looks good to me.
>
> Best regards
> Christoph
>
> > -----Original Message-----
> > From: hotspot-runtime-dev <hotspot-runtime-dev-
> > bounces at openjdk.java.net> On Behalf Of Thomas Stüfe
> > Sent: Mittwoch, 22. Mai 2019 11:25
> > To: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>; David
> > Holmes <david.holmes at oracle.com>
> > Subject: Re: 8220394: bufferedStream does not honor size limit
> >
> > 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
> > >>
> > >>
> > >>
>