RFR (L): 8062370: Various minor code improvements

[Thomas Stüfe]

Hi David,

Our intend was to always guarantee that the written string is zero
terminated, and across platforms to always return the same value (-1) if
truncation happened.

The original jio_snprintf() did not have any value over plain snprintf()
(apart from maybe solving the name problem with "_snprintf" on Windows).
But having a dedicated wrapper function around snprintf() suggests some
added value, and I always thought that value was supposed to be zero
termination. If that is not intended, it would be better to use the plain
snprintf instead, because at least C programmers then know what to expect.

I also found very few cases where the return code of jio_snprintf() was
actually checked and truncation handled correctly. Which would be difficult
too, because the return code differed for truncation between windows and
Posix.

Bottomline, I think it would be better if jio_snprintf() were to always
zero-terminate, guaranteed.

Kind Regards, Thomas


On Thu, Nov 6, 2014 at 12:23 PM, David Holmes <david.holmes at oracle.com>
wrote:

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