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

[Ioi Lam]

On Thu, 28 Apr 2022 08:08:42 GMT, Johan Sjölén <duke at openjdk.java.net> wrote:

>> os::vsnprintf can return a negative value on encoding error. A negative return value will cause wraparound when cast to size_t. This in turn causes LogMessageBuffer::grow() to attempt a large memory allocation. Instead of accepting this we bail on the logging.
>
> Johan Sjölén has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Move va_end to avoid duplication

Changes requested by iklam (Reviewer).

src/hotspot/share/logging/logMessageBuffer.cpp line 115:

> 113:     int ret = os::vsnprintf(current_buffer_position, remaining_buffer_length, fmt, copy);
> 114:     va_end(copy);
> 115:     assert(ret >= 0, "Log message buffer issue.");

According to https://www.cplusplus.com/reference/cstdio/vsnprintf/ , the only reason for vsnprintf returning a negative value is when there's an encoding error.

Here's an example
https://stackoverflow.com/questions/28096802/what-is-the-meaning-of-encoding-error-in-the-vsnprintf-documentation

@jdksjolen have you seen an actual case where the JVM failed due to this bug?

I am not sure if we should have the assert. I think the assert will fail for cases like this:


./share/classfile/javaClasses.cpp:
log_error(class)("Mismatch JDK version for field: %s type: %s",
     name_symbol->as_C_string(), signature_symbol->as_C_string());


`as_C_string()` returns a string in UTF8 encoding. If the `name_symbol` contains non-ascii characters, the string may not be compatible with the JVM's current locale.

Klass::external_name() has a similar problem:


./share/classfile/verifier.cpp:
    log_info(verification)("Verifying class %s with old format",
                            klass->external_name());


Maybe it's a good idea to have the assert so we can fix these, but there are at least a few dozen cases in our code.

So I would suggest leaving the assert out of this PR, and perhaps do it in a separate RFE.

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

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