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

[Thomas Stuefe]

On Fri, 29 Apr 2022 05:04:24 GMT, David Holmes <dholmes at openjdk.org> wrote:

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

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.

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

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