RFR (L): 8062370: Various minor code improvements

Thu Nov 13 03:03:49 UTC 2014

Hi Thomas,

On 12/11/2014 8:31 PM, Thomas Stüfe wrote:
> Hi,
>
> could you please review this little addition? (added comments for
> jio_snprintf)
>
> http://cr.openjdk.java.net/~simonis/webrevs/8062370/

A new bug is needed for these changes.

As people rarely look at the header file when reading the code could you 
augment the last line of the comment in jvm.cpp from:

+   // return always -1.

to

+   // always return -1, and perform null termination.

Thanks,
David

> Thanks!
>
> Best Regards,
>
> Thomas
>
> On Tue, Nov 11, 2014 at 11:37 AM, Markus Grönlund <
> markus.gronlund at oracle.com> wrote:
>
>> Hi Goetz,
>>
>> Thanks for following up on this.
>>
>> I adjusted a few calls into jio_snprintf() that were particular for
>> Windows to accommodate the updates.
>>
>>  From the test results I have seen so far, it seems no other issues
>> appeared which could be related to this.
>>
>> So I think the change should be left as is (no rollback), but maybe a
>> comment could be added about the jio_snprintf() semantics (NULL termination
>> on overflows, expected 'count' etc).
>>
>> Thanks
>> Markus
>>
>>
>> -----Original Message-----
>> From: Lindenmaier, Goetz [mailto:goetz.lindenmaier at sap.com]
>> Sent: den 11 november 2014 11:14
>> To: Markus Grönlund
>> Cc: David Holmes; hotspot-dev at openjdk.java.net
>> Subject: RE: RFR (L): 8062370: Various minor code improvements
>>
>> Hi Markus,
>>
>> Could you fix your issue and did the other tests pass?
>>
>> Are there any follow-up actions I should take?
>>
>> Best regards,
>>    Goetz.
>>
>> -----Original Message-----
>> From: Markus Grönlund [mailto:markus.gronlund at oracle.com]
>> Sent: Donnerstag, 6. November 2014 14:50
>> To: Lindenmaier, Goetz
>> Cc: David Holmes; hotspot-dev at openjdk.java.net
>> Subject: RE: RFR (L): 8062370: Various minor code improvements
>>
>> Hi Goetz,
>>
>> Thanks for looking into this.
>>
>> I think I will be able to update the internal code I am working on to
>> accommodate your updates.
>>
>> I don't know if any other code will see potential issues - only testing
>> will tell.
>>
>> So I would await the rollback and I will putback my updated code - let's
>> see if other issues appear after this - we should know after this nights
>> nightly testing (then we can re-evaluate the rollback).
>>
>> Thanks
>> Markus
>>
>>
>> -----Original Message-----
>> From: Lindenmaier, Goetz [mailto:goetz.lindenmaier at sap.com]
>> Sent: den 6 november 2014 13:49
>> To: David Holmes; hotspot-dev at openjdk.java.net
>> Cc: Markus Grönlund
>> Subject: RE: RFR (L): 8062370: Various minor code improvements
>>
>> Hi David,
>>
>> Well, yes, that's right.  But then you can simply pass in count+1.
>> It works also if the caller knows he will only use 'count' bytes of the
>> string.  In this case +1 must be allocated.
>> But that both is quite special.
>>
>> Currently, if the string is truncated, there is no null byte on windows.
>> And there are a lot of uses of this method in the VM (via jio_snprintf).
>>
>> Should I use the internal bug number for the rollback-fix?
>>
>> How should we proceed, as I can't fix you internal code?
>>
>> Best regards,
>>    Goetz.
>>
>>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Donnerstag, 6. November 2014 12:23
>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
>> Cc: Markus Grönlund
>> Subject: Re: RFR (L): 8062370: Various minor code improvements
>>
>> On 6/11/2014 8:43 PM, Lindenmaier, Goetz wrote:
>>> Hi David,
>>>
>>> yes, windows does not null terminate if there is an overflow.
>>> Obviously there are overflows, and they now see one less character.  I
>>> think this should be fixed where jio_vsnprintf is called.  Having
>>> non-null terminated strings isn't nice.
>>
>> I think it depends on what you consider an overflow. If the buffer is
>> already null terminated and you pass in a count that covers up to the
>> location before the null then there is no problem - except now the logic
>> will introduce a second null in place of the last character.
>>
>>> But for now I will roll back this single change.  I'll send a RFR soon.
>>>
>>> Where did you see the problem?
>>
>> It was in our closed code so I can't go into details. We have a non-public
>> bug number: 8063089
>>
>> Thanks,
>> David
>>
>>>
>>> Best regards,
>>>     Goetz.
>>>
>>>
>>>
>>>
>>> -----Original Message-----
>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>> Sent: Donnerstag, 6. November 2014 11:30
>>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
>>> Cc: Markus Grönlund
>>> Subject: Re: RFR (L): 8062370: Various minor code improvements
>>>
>>> On 6/11/2014 8:17 PM, Lindenmaier, Goetz wrote:
>>>> Thanks David, I'll have a look.
>>>
>>> It seems that windows vsnprintf may not null-terminate the string -
>>> which I think is what your patch was trying to address. But if we have
>>> existing code that works with that then the fix is now overwriting the
>>> last character. I can't quite see how to handle this in a cross
>>> platform manner, but in the immediate term we should probably revert
>>> that part of the changeset.
>>>
>>> David
>>>
>>>> Best regards,
>>>>      Goetz.
>>>>
>>>> -----Original Message-----
>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>> Sent: Donnerstag, 6. November 2014 11:09
>>>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
>>>> Cc: Markus Grönlund
>>>> Subject: Re: RFR (L): 8062370: Various minor code improvements
>>>>
>>>> Hi Goetz,
>>>>
>>>> This change has introduced a bug:
>>>>
>>>> -  return vsnprintf(str, count, fmt, args);
>>>> +
>>>> +  int result = vsnprintf(str, count, fmt, args);  if ((result > 0 &&
>>>> + (size_t)result >= count) || result == -1) {
>>>> +    str[count - 1] = '\0';
>>>> +    result = -1;
>>>> +  }
>>>> +
>>>> +  return result;
>>>>
>>>> some strings are getting their last character truncated on Windows.
>>>>
>>>> David
>>>>
>>>> On 5/11/2014 6:16 PM, Lindenmaier, Goetz wrote:
>>>>> Hi David,
>>>>>
>>>>> thanks for looking at the change!  I fixed the issue in a new
>>>>> webrev:
>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8062370/webrev.01/
>>>>>
>>>>> Best regards,
>>>>>       Goetz.
>>>>>
>>>>> -----Original Message-----
>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>>> Sent: Mittwoch, 5. November 2014 02:49
>>>>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
>>>>> Subject: Re: RFR (L): 8062370: Various minor code improvements
>>>>>
>>>>> Hi Goetz,
>>>>>
>>>>> The only issue I see is in:
>>>>>
>>>>> src/share/vm/runtime/globals.cpp
>>>>>
>>>>> where you replaced NEW_C_HEAP_ARRAY with os::strdup. To keep the
>>>>> "abort on OOM" semantics of NEW_C_HEAP_ARRAY you need to use
>> os::strdup_check_oom.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>> On 30/10/2014 6:28 PM, Lindenmaier, Goetz wrote:
>>>>>> Hi,
>>>>>>
>>>>>> this change contains a row of minor code improvements we did to
>>>>>> fulfil our internal quality requirements.  We would like to share
>>>>>> these with openJDK.
>>>>>>
>>>>>> Please review and test this change.  I please need a sponsor.
>>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8062370/webrev.00/
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8062370
>>>>>>
>>>>>> We tested this on windows 64, linux x86_64, mac, solaris sparc
>>>>>> 32+64 bit and, of course, the ppc platforms.
>>>>>>
>>>>>>
>>>>>> Some details:
>>>>>>
>>>>>> CONST64(0x8000000000000000) is wrong, as 0x8... is positive, and thus
>> not representable as i64 what is used in the CONST64 macro. This change
>> adapts UCONST64 to use ui64, and the usages of these macros where necessary.
>>>>>>
>>>>>> We add some more strncpy uses.  Also, we fix strncpy on windows.
>> There, strncpy does not write a \0 into the last byte if the copied string
>> is too long.
>>>>>>
>>>>>> We add some missing memory frees and some closing of files.
>>>>>>
>>>>>> jio_vsnprintf() works differently on windows and linux. This change
>> adapts this to show the same behaviour on all platforms. See java.cpp.
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>>        Goetz
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>