RFR: JDK-8285712: LogMessageBuffer doesn't check vsnprintf return value [v2]

[Johan Sjölén]

On Fri, 29 Apr 2022 05:05:18 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Posix does potentially define a few other errors cases but the encoding error is what we ourselves document.
>> 
>> This is a good point @iklam. If  non-ascii class names could lead to problems then UL is far more fragile than I thought it to be! But I still think the assert is a good idea to detect this problem during development and testing as presumably during development and testing we don't want log messages to be silently dropped because of this encoding problem. That said we need to ensure we don't have existing tests that would cause this to trigger in the CI testing.
>
> Before, the VM would assumedly fail with a confusing native OOM. I cannot see how the subsequent malloc could have succeeded. So arguably, an assert is not worse, but clearer.
> 
> OTOH, this is "only" logging. We could just write something like "ENCODINGERROR" into the message buffer instead, to trip people off. Better than just to leave out the printed string. Maybe we should do this in release builds anyway, regardless whether we assert out in debug.

The other calls to `os::vsnprintf` does the same check and assert. I assume that we should have seen assertions failing by 
now, if the non-ASCII/non-compliant locale issue actually occurred.

@iklam, your link to StackOverflow links to this in-depth answer: https://groups.google.com/g/comp.std.c/c/lIvkxXr5_wE/m/aVuIpm52ToQJ (read James Kuyper's message). The TL;DR is that this is an issue exactly with `%ls` and `%lc` which are used for support with `wchar_t`.

Example of other site where `os::vsnprintf` is checked.


// logTagSet.cpp:130
  // Check that string fits in buffer; resize buffer if necessary
  int ret;
  if (prefix_len < vwrite_buffer_size) {
    ret = os::vsnprintf(buf + prefix_len, sizeof(buf) - prefix_len, fmt, args);
  } else {
    // Buffer too small. Just call printf to find out the length for realloc below.
    ret = os::vsnprintf(nullptr, 0, fmt, args);
  }

  assert(ret >= 0, "Log message buffer issue");
  if (ret < 0) {
    // Error, just log contents in buf.
    log(level, buf);
    log(level, "Log message buffer issue");
    va_end(saved_args);
    return;
  }


@tstuefe, I agree, and so does the example above. I will be adding a log message to this.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8423