RFR (XS): 8218937: Make mlvmJvmtiUtils strncpy uses GCC 8.x friendly

[Mikael Vidstedt]

Thanks Igor. The :vmTestbase_vm_mlvm testing passed, so I went ahead and pushed the change.

JC/Igor, I’m taking suggestions on where to potentially move the new JvmtiResource class, let me know!

Cheers,
Mikael

> On Feb 13, 2019, at 5:49 PM, Igor Ignatyev <igor.ignatyev at oracle.com> wrote:
> 
> 
> 
>> On Feb 13, 2019, at 5:36 PM, Mikael Vidstedt <mikael.vidstedt at oracle.com <mailto:mikael.vidstedt at oracle.com>> wrote:
>>> On Feb 13, 2019, at 5:17 PM, Igor Ignatyev <igor.ignatyev at oracle.com <mailto:igor.ignatyev at oracle.com>> wrote:
>>> strncpy(mn->methodName, szName, sizeof(mn->methodName) -1) is guaranteed to be null-terminated, it might produce truncated string if methodName is shorter than szName.
>> 
>> Unfortunately not. From man strncpy:
>> 
>> "Warning: If there is no null byte among the first  n  bytes  of  src,  the  string placed in dest will not be null-terminated.”
>> 
>> So no matter the length we’ll risk having an unterminated string.
> so much for a safe function.
> 
>> 
>>> I'm actually not suggesting to replace the if you added, I'm suggesting to also add -1 to both strncpy, so gcc will be happy even if it becomes less smart.
>> 
>> I took a stab at doing this "the C++ way”. Have a look and let me know what you think:
>> 
>> http://cr.openjdk.java.net/~mikael/webrevs/8218937/webrev.01/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8218937/webrev.01/open/webrev/>
> LGTM. I do recall JC (cc'ed) working on "shared" helper classes for JVMTI/JNI, it might be worth it to move your JvmtiResource closer to them, but this can/should be done by a separate RFE.
> 
> -- Igor
>  
>> 
>> Cheers,
>> Mikael
>> 
>>> 
>>> -- Igor
>>> 
>>>> On Feb 13, 2019, at 5:10 PM, Mikael Vidstedt <mikael.vidstedt at oracle.com <mailto:mikael.vidstedt at oracle.com>> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Feb 13, 2019, at 4:48 PM, Igor Ignatyev <igor.ignatyev at oracle.com <mailto:igor.ignatyev at oracle.com>> wrote:
>>>>> 
>>>>> > One could imagine wrapping the resource in a C++ class and have the destructor do the deallocation.
>>>>> that sounds nice.
>>>>> 
>>>>>>      strncpy(mn->methodName, szName, sizeof(mn->methodName));
>>>>>>      strncpy(mn->classSig, szSignature, sizeof(mn->classSig));
>>>>> as we are doing this just to make gcc happier, wouldn't it be better to add -1 to sizeof?
>>>> 
>>>> While that does silence the warning, it (still) has the issue of potentially leaving the resulting strings unterminated. We may know today that the strings in question are always shorter than the respective MethodName buffers, but it seems prudent to actually perform the checks to ensure that.
>>>> 
>>>> Cheers,
>>>> Mikael
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> -- Igor
>>>>> 
>>>>> 
>>>>>> On Feb 13, 2019, at 4:38 PM, Mikael Vidstedt <mikael.vidstedt at oracle.com <mailto:mikael.vidstedt at oracle.com>> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>> On Feb 13, 2019, at 4:12 PM, Igor Ignatyev <igor.ignatyev at oracle.com <mailto:igor.ignatyev at oracle.com>> wrote:
>>>>>>> 
>>>>>>> Hi Mikael,
>>>>>>> 
>>>>>>> I really don't like us adding goto in cpp code, can we avoid that?
>>>>>> 
>>>>>> One alternative is to make sure the Deallocate calls are injected in every if statement. I don’t think that’s any better.
>>>>>> 
>>>>>> Another alternative is to nest the if statements. I’m not a fan of that code style personally.
>>>>>> 
>>>>>> One could imagine wrapping the resource in a C++ class and have the destructor do the deallocation.
>>>>>> 
>>>>>> Let me know if you have other suggestions, and/or what you’d prefer.
>>>>>> 
>>>>>>> I also doubt that any of mlvm tests are run in tier1, could you please also run :vmTestbase_vm_mlvm to test your fix?
>>>>>> 
>>>>>> Will do.
>>>>>> 
>>>>>> Cheers,
>>>>>> Mikael
>>>>>> 
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> -- Igor
>>>>>>> 
>>>>>>> 
>>>>>>>> On Feb 13, 2019, at 3:45 PM, Mikael Vidstedt <mikael.vidstedt at oracle.com <mailto:mikael.vidstedt at oracle.com>> wrote:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Please review this small fix which updates some uses of strncpy to address some GCC 8.x warnings.
>>>>>>>> 
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8218937 <https://bugs.openjdk.java.net/browse/JDK-8218937>
>>>>>>>> Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8218937/webrev.00/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8218937/webrev.00/open/webrev/>
>>>>>>>> 
>>>>>>>> GCC 8.2 is producing a warning for mlvmJvmtiUtils.cpp: 
>>>>>>>> 
>>>>>>>> In file included from test/hotspot/jtreg/vmTestbase/vm/mlvm/indy/func/jvmti/share/libIndyRedefineClass.cpp:31: 
>>>>>>>> test/hotspot/jtreg/vmTestbase/vm/mlvm/share/mlvmJvmtiUtils.cpp: In function 'MethodName* getMethodName(jvmtiEnv*, jmethodID)': 
>>>>>>>> test/hotspot/jtreg/vmTestbase/vm/mlvm/share/mlvmJvmtiUtils.cpp:80:12: error: 'char* strncpy(char*, const char*, size_t)' specified bound 256 equals destination size [-Werror=stringop-truncation] 
>>>>>>>>      strncpy(mn->methodName, szName, sizeof(mn->methodName)); 
>>>>>>>>      ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>> 
>>>>>>>> Basically, gcc is pointing out that the resulting string is not necessarily going to be terminated. Explicitly checking the length of the source strings provides the information needed to guarantee that the strings will be terminated, and silences the warning.
>>>>>>>> 
>>>>>>>> Passes tier1.
>>>>>>>> 
>>>>>>>> Cheers,
>>>>>>>> Mikael
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20190214/3b1508cd/attachment.html>