RFR: 8232084: HotSpot build failed with GCC 9.2.1

[Chris Plummer]

I thought it was because gcc sees that "len" is always passed in as 
strlen(str). It therefore knows without any doubt that you are doing 
strncpy() on a nul terminated string, and are copying all but the nul. 
However, I missed one case where parse_value() is called and len not 
only is not computed by strlen(), but the string is a delimited one, not 
nul terminated:

void GenDCmdArgument::read_value(const char* str, size_t len, TRAPS) {
...
   parse_value(str, len, CHECK);
   set_is_set(true);
}

Look at calls to read_value and you'll see it is a delimited string. 
This seems to be the path for parsing actual specified arguments. The 
other calls to parse_value() are all for default values, which are 
always a single nul terminated string.

What's also odd is that the code for DCmdArgument<jlong>::parse_value() 
is identical to DCmdArgument<jboolean>::parse_value() w.r.t. the 
strncpy(), but does not produce a warning.

Chris

On 10/10/19 10:29 AM, Ioi Lam wrote:
> 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)
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>
>>
>