RFR: 8232084: HotSpot build failed with GCC 9.2.1

[Ioi Lam]

We have strncpy all over the VM. Why is this the only one that's causing 
problems with gcc?

We have one instance in the same file:

void StringArrayArgument::add(const char* str, size_t len) {
   if (str != NULL) {
     char* ptr = NEW_C_HEAP_ARRAY(char, len+1, mtInternal);
     strncpy(ptr, str, len);
     ptr[len] = 0;
     _array->append(ptr);
   }
}

that looks rather similar to the troubling case:

     char* buf = NEW_RESOURCE_ARRAY(char, len + 1);
     strncpy(buf, str, len);
     buf[len] = '\0';

To me, this seems like a bug in gcc 9.2.1, which is the cutting edge of 
gcc releases. Maybe we should just avoid supporting gcc 9.2 until this 
has been fixed?

Thanks
- Ioi



On 10/10/19 8:58 AM, Chris Plummer wrote:
> On 10/10/19 1:07 AM, Yasumasa Suenaga wrote:
>> On 2019/10/10 16:42, David Holmes wrote:
>>> On 10/10/2019 5:25 pm, serguei.spitsyn at oracle.com wrote:
>>>> On 10/9/19 23:39, Chris Plummer wrote:
>>>>> I think the +1 gets rid of the warning, but it also means there is 
>>>>> no need for the following line that sets buf[len].
>>>>
>>>> Yes, it was my guess.
>>>> Then I agree this statement must be removed: buf[len] = '\0';
>>>
>>> I don't think so!
>>>
>>> Given this description:
>>>
>>> template <> void DCmdArgument<bool>::parse_value(const char* str,
>>>                                                   size_t len, TRAPS) {
>>>    // len is the length of the current token starting at str
>>>
>>> it is not evident to me that we are even dealing with a 
>>> NUL-terminated str. Rather str is the start of the next token, with 
>>> length len, after which may be another token and so forth. So we may 
>>> be passing in:
>>>
>>> str = "token1 token2"
>>> len = 6
>>>
>>> so we still need the explicit buf[len]='\0'
>>
>> I missed it, thanks!
>> I think we can fix it as below.
>>
>> -------------
>> diff -r b4f2e13d20ea src/hotspot/share/services/diagnosticArgument.cpp
>> --- a/src/hotspot/share/services/diagnosticArgument.cpp Wed Oct 09 
>> 19:38:11 2019 -0700
>> +++ b/src/hotspot/share/services/diagnosticArgument.cpp Thu Oct 10 
>> 17:04:09 2019 +0900
>> @@ -151,7 +151,7 @@
>> ResourceMark rm;
>>
>>        char* buf = NEW_RESOURCE_ARRAY(char, len + 1);
>> -      strncpy(buf, str, len);
>> memcpy(buf, str, len);
>> buf[len] = '\0';
>>        Exceptions::fthrow(THREAD_AND_LOCATION, 
>> vmSymbols::java_lang_IllegalArgumentException(),
>>          "Boolean parsing error in command argument '%s'. Could not 
>> parse: %s.\n", _name, buf);
>> -------------
>>
>> I will upload webrev if it is ok.
> That looks right, although it's odd that the memcpy line does not have 
> a + in front of it to show that it's an addition.
>
> Chris
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>> David
>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>>
>>>>> Chris
>>>>>
>>>>> On 10/9/19 11:31 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Yasumasa,
>>>>>>
>>>>>> The fix in src/hotspot/cpu/x86/macroAssembler_x86.hpp looks Okay.
>>>>>>
>>>>>> But I don't understand why you need this fix in 
>>>>>> src/hotspot/share/services/diagnosticArgument.cpp ?:
>>>>>>         char* buf = NEW_RESOURCE_ARRAY(char, len + 1);
>>>>>> - strncpy(buf, str, len);
>>>>>> + strncpy(buf, str, len + 1);
>>>>>>         buf[len] = '\0';
>>>>>> The buf[len] is set to '\0' anyway.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 10/9/19 22:55, Yasumasa Suenaga wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Please review this change:
>>>>>>>
>>>>>>>   JBS: https://bugs.openjdk.java.net/browse/JDK-8232084
>>>>>>>   webrev: 
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.00/
>>>>>>>
>>>>>>>
>>>>>>> I tried to build OpenJDK on Fedora 30 x64 with GCC 9.2.1, but it 
>>>>>>> was failed in
>>>>>>> macroAssembler_x86.hpp and diagnosticArgument.cpp .
>>>>>>> (Please see JBS for details)
>>>>>>>
>>>>>>> This change has passed tests on submit repo.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa (ysuenaga)
>>>>>>
>>>>>
>>>>>
>>>>
>
>