RFR (L): 8062370: Various minor code improvements

[Lindenmaier, Goetz]

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