RFR (L): 8062370: Various minor code improvements

Thu Nov 6 20:33:56 UTC 2014

Hi Markus,

On 6/11/2014 11:50 PM, Markus Grönlund wrote:
> 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 for dealing with this without needing to do a partial rollback of 
the current changes.

We need to do a quick audit of windows-only uses of jio_snprintf to 
check that other code is not also expecting the Windows vsnprintf semantics.

We will also close the pre-push testing gap that allowed this to slip 
through.

Thanks,
David

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