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

[David Holmes]

On 12/08/2016 1:05 PM, Chris Plummer wrote:
> 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.

AFAICS there are no other modified uses in:

http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/110ec5963eb1#l23.1

If this change could have affected other uses of jio_snprintf then I 
would have expected them to be covered in that patch. If it didn't do 
the right thing and that is a bug in the 9 code then it needs to be 
flagged in 9, fixed and then backported.

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

Yes, I think that would be a minimal fix and avoid the ripple effects 
that any other change would make.

> return value to be the actual number of bytes written. That should fix

Yes that may have been a better choice - especially for 9 going forward 
(as of VS2015 Windows vsnprintf behaves the same as POSIX (C99 standard 
conforming)). But that choice wasn't made in 9 and we really shouldn't 
have 8 and 9 differ unnecessarily. Plus it would still require checking 
all uses.

Cheers,
David

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