RFR (L): 8062370: Various minor code improvements

Tue Nov 11 10:13:37 UTC 2014

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