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

[David Holmes]

On Fri, 29 Apr 2022 04:51:15 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Johan Sjölén has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Move va_end to avoid duplication
>
> 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.

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.

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

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