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

[Ioi Lam]

On Fri, 29 Apr 2022 07:48:21 GMT, Johan Sjölén <duke at openjdk.java.net> wrote:

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

I ran with `java -Xlog:all=debug --version`.  `vsnprintf()` was called 8495 times in `LogTagSet::vwrite()` and 746 times in `LogMessageBuffer::vwrite()`. So I agree with @jdksjolen that having an assert in the new code doesn't make the problem any worse than it's today.

We don't see the assert for a variety of reasons
- The VM currently doesn't use `%ls` and `%lc` in unified logging.
- Most people run Java under UTF8 locales.
- Most Java class/method/field names are ascii-only.

So we can probably leave things as is.

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

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