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

[David Holmes]

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.

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.

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