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

[Chris Plummer]

On 8/11/16 11:07 PM, David Holmes wrote:
> 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.
If that's the case then maybe that part of the changeset should be 
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.
Ok. I see that there are lots of places that already check for "ret < 0 
|| buflen < ret", so it looks like the Windows difference is being 
special cased in shared code.

cheers,

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