[8u] RFR for JDK-8162419: closed/com/oracle/jfr/runtime/TestVMInfoEvent.sh failing after JDK-8155968

[Chris Plummer]

On 8/11/16 5:42 PM, David Holmes wrote:
> Hi Chris,
>
> On 12/08/2016 9:20 AM, Chris Plummer wrote:
>> Hi Shafi,
>>
>> Please update the copyright date to 2016 and change "numbers of" to
>> "number of".
>>
>> I'm not so sure I agree with the comments in the CR that you can just
>> backport this change to vsnprintf(), but not the other changes in the
>> relevant changeset. For example:
>>
>> --- a/src/share/vm/runtime/java.cpp    Mon Nov 03 11:34:13 2014 -0800
>> +++ b/src/share/vm/runtime/java.cpp    Wed Oct 29 10:13:24 2014 +0100
>> @@ -705,25 +705,35 @@
>>  }
>>
>>  void JDK_Version::to_string(char* buffer, size_t buflen) const {
>> +  assert(buffer && buflen > 0, "call with useful buffer");
>>    size_t index = 0;
>> +
>>    if (!is_valid()) {
>>      jio_snprintf(buffer, buflen, "%s", "(uninitialized)");
>>    } else if (is_partially_initialized()) {
>>      jio_snprintf(buffer, buflen, "%s", "(uninitialized) pre-1.6.0");
>>    } else {
>> -    index += jio_snprintf(
>> +    int rc = jio_snprintf(
>>          &buffer[index], buflen - index, "%d.%d", _major, _minor);
>> +    if (rc == -1) return;
>> +    index += rc;
>>      if (_micro > 0) {
>> -      index += jio_snprintf(&buffer[index], buflen - index, ".%d",
>> _micro);
>> +      rc = jio_snprintf(&buffer[index], buflen - index, ".%d", _micro);
>>      }
>>
>> I think your change to vsnprintf() will break JDK_Version::to_string()
>> if the above diff if not applied. You could argue that the above code is
>> already broken because -1 is could be returned to it on Windows.
>> However, your changes expand that risk to all platforms.
>
> Unless I am missing something that risk only exists if there are 
> absurd values in the version components. That would only affect 
> someone building with their own version values defined.
Hi David,

That was just the first example I found of an affected jio_snprintf(). 
I'm guessing there are others.
>
> So yes this code could also be more robust, but it isn't essential for 
> fixing the issue at hand that needed fixing - which was the missing 
> nul-terminator when a too-short buffer is passed in.
So why not just fix the null termination, but not change the return 
value to -1. In fact the correct thing to do here would be to change the 
return value to be the actual number of bytes written. That should fix 
existing code like JDK_Version::to_string() that does not expect to see 
the -1.

cheers,

Chris
>
> Cheers,
> David
>
>> cheers,
>>
>> Chris
>>
>> On 8/11/16 5:14 AM, Shafi Ahmad wrote:
>>> Hi,
>>>
>>> Could I get one more review for this safe change.
>>>
>>> Regards,
>>> Shafi
>>>
>>>> -----Original Message-----
>>>> From: David Holmes
>>>> Sent: Thursday, August 11, 2016 9:52 AM
>>>> To: Shafi Ahmad; hotspot-runtime-dev at openjdk.java.net
>>>> Subject: Re: [8u] RFR for JDK-8162419:
>>>> closed/com/oracle/jfr/runtime/TestVMInfoEvent.sh failing after JDK-
>>>> 8155968
>>>>
>>>> Hi Shafi,
>>>>
>>>> On 10/08/2016 6:34 PM, Shafi Ahmad wrote:
>>>>> Hi,
>>>>>
>>>>> Please review the code change for "JDK-8162419:
>>>> closed/com/oracle/jfr/runtime/TestVMInfoEvent.sh failing after JDK-
>>>> 8155968" to jdk8u.
>>>>> Please note this is partial backport of
>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/110ec5963eb1#l23.1
>>>>> Summary:
>>>>> Microsoft version of vsnprintf() behaves differently from the
>> standard C
>>>> version when there is not enough space in the buffer.
>>>>> Microsoft version doesn't null terminates its output under error
>> conditions,
>>>> whereas the standard C version does. On Windows, it returns -1.
>>>>> We handle both cases here and always return -1, and perform null
>>>> termination.
>>>>
>>>> This looks fine to me.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Jdk8 bug: https://bugs.openjdk.java.net/browse/JDK-8162419
>>>>> Webrev link: http://cr.openjdk.java.net/~shshahma/8162419/webrev.00/
>>>>>
>>>>> Testing: jprt
>>>>>
>>>>> Regards,
>>>>> Shafi
>>>>>
>>
>>