RFR(XL): 8181917: Refactor UL LogStreams to avoid using resource area

[Thomas Stüfe]

Hi Erik!

On Mon, Jul 10, 2017 at 11:40 AM, Erik Helin <erik.helin at oracle.com> wrote:

> Hi Thomas,
>
> On 06/30/2017 08:32 PM, Thomas Stüfe wrote:
>
>> Hi Eric,
>>
>> thank you for the review!
>>
>> New
>> Version: http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-
>> ul-logstream/all.webrev.03/webrev/
>>
>
> I think you still have a problem with "runaway" loggers in this version:
>
> +// try_ensure_cap tries to enlarge the capacity of the internal buffer
> +// to the given atleast value. May fail if either OOM happens or atleast
> +// is larger than a reasonable max of 1 M. Caller must not assume
> +// capacity without checking.
> +void LogStream::LineBuffer::try_ensure_cap(size_t atleast) {
> +  assert(_cap >= sizeof(_smallbuf), "sanity");
> +  if (_cap < atleast) {
> +    const size_t reasonable_max = 1 * M;
> +    size_t newcap = align_size_up(atleast + 64, 64);
> +    assert(_cap <= reasonable_max, "sanity");
> +    // Cap out at a reasonable max to prevent runaway leaks.
> +    if (newcap > reasonable_max) {
> +      newcap = reasonable_max;
> +    }
> +
> +    char* const newbuf = (char*) os::malloc(newcap, mtLogging);
> +    if (newbuf == NULL) { // OOM. Leave object unchanged.
> +      return;
> +    }
> +    if (_pos > 0) { // preserve old content
> +      memcpy(newbuf, _buf, _pos + 1); // ..including trailing zero
> +    }
> +    if (_buf != _smallbuf) {
> +      os::free(_buf);
> +    }
> +    _buf = newbuf;
> +    _cap = newcap;
> +  }
> +  assert(_cap >= atleast, "sanity");
> +}
>
> with the above code, even though _cap > reasonable_max, you will always
> allocate newbuf with os::malloc. For debug builds, we are fine, since the
> JVM will assert on _cap <= reasonable_max, but for product builds we are
> not. Would it be better to first check if _cap == reasonable_max? As in the
> following snippet:
>
> void LogStream::LineBuffer::try_ensure_cap(size_t atleast) {
>   if (_cap < atleast) {
>     const size_t reasonable_max = 1 * M;
>     if (_cap == reasonable_max) {
>       // max memory used, "die" in debug builds, log in product builds
>       log_warning(logging)("Max memory used for message: %s\n", _buf);
>       ShouldNotReachHere();
>       return;
>     }
>
>     size_t newcap = align_size_up(atleast + 64, 64);
>     if (newcap > reasonable_max) {
>       newcap = reasonable_max;
>     }
>     char* const newbuf = (char*) os::malloc(newcap, mtLogging);
>     // rest of logic
>   }
> }
>
> Thanks,
> Erik
>

I'm not sure I understand. Are you concerned that _cap would grow beyond
reasonable_max - because I do not see a way this could happen. Or do you
want to also treat the *attempt* to grow the _cap beyond reasonable_max as
a loggable offence?

If the latter, I am not sure about logging. If this is an error which may
happen at a customer site without popping up first in inhouse-tests (which
it conceivably is, because logging is very content dependent), I would
rather hide the error in production code instead of flooding stderr with
tons of log warnings or crashing with a guarantee, no?

..Thomas