8220394: bufferedStream does not honor size limit

[Thomas Stüfe]

Hi David,

thank you for taking a look at this! Remarks inline:

On Tue, May 21, 2019 at 8:45 AM David Holmes <david.holmes at oracle.com>
wrote:

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


Definitely.


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

Yes, but the problem is that until it finally sends the output of an attach
op back it completely buffers the whole output in the stream internal
buffer. That is a problem especially if you have faulty printing code.

I encountered the problem when I was working on a diagnostic command and
had an error resulting in infinite printing. The process was growing until
the OOM killer killed it.

The intention of my patch is not to fix those usages - that should be done
separately, e.g. we should teach the attach listener to send output
piecemeal, or temporarily buffer to a local file. But that would be a much
larger fix, and only fixes this one occurrence.

What I wanted was just an assert at a size which reasonably could be called
an error. Would make analyzing these issues much easier. And since this is
an error which very likely depends on runtime conditions (e.g. you may hit
an error in a jcmd at a customer site you never saw in lab) it makes sense
to have a solution for release builds too, hence the silent truncation.

Do you have a better proposal?


>
> So where is the code that misuses bufferedStream?
>
>
It was jcmd.


> Thanks,
> David
> -----
>

Cheers, Thomas


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