RFR [XS]: 8239462: jdk.hotspot.agent misses some ReleaseStringUTFChars calls in case of early returns

[Alex Menkov]

On 02/21/2020 00:09, Baesken, Matthias wrote:
>> Also I see GetStringUTFChars(str, JNI_FALSE). This look bad as well -
>> 2nd arg is a pointer, so it should be NULL or nullptr.
> 
> Hi looks like there is another one here, do you think these JNI_FALSE params would  really cause trouble ? Not even the compiler warns here ...
> 
> src/java.desktop/unix/native/libawt_xawt/xawt/XlibWrapper.c:824:        cname = (char *) (*env)->GetStringUTFChars(env, jstr, JNI_FALSE);

This doesn't cause troubles just because both NULL & JNI_FALSE are 
defines (and are 0). but specify JNI_FALSE as pointer value is confusing.

--alex

> 
> Best regards, Matthias
> 
> 
> 
>> Hi Matthias,
>>
>> Looks good in general, but I think it makes sense to fix #2 cases (at
>> least I see them in LinuxDebuggerLocal). If GetStringUTFChars fails, the
>> code will crash.
>> Also I see GetStringUTFChars(str, JNI_FALSE). This look bad as well -
>> 2nd arg is a pointer, so it should be NULL or nullptr.
>>
>> As for #1 and #3 - AFAIU they are both right ways.
>> If GetStringUTFChars fails, it throws OOM and return NULL.
>>
>> And one more thing to consider.
>> LinuxDebuggerLocal_attach0 function looks terrible - 7
>> ReleaseStringUTFChars calls for 2 GetStringUTFChars.
>> Maybe it make sense to introduce simple wrapper like AutoJavaString in
>> src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp
>> It would make the code simpler and less error prone.
>>
>> --alex
>>
>> On 02/20/2020 00:53, Baesken, Matthias wrote:
>>> Hi Alex / Christoph,  thanks for the reviews.
>>>
>>> New webrev :
>>>
>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8239462.1/
>>>
>>> - includes  LinuxDebuggerLocal.cpp
>>> - adds a blank Christoph wanted to have
>>>
>>>
>>> A question (hopefully not a stupid one �� ):
>>> At most places in the coding,  GetStringUTFChars success is
>>> 1. handled by checking NULL , like this :
>>>
>>> const char *s = (*env)->GetStringUTFChars(env, p, NULL);
>>> if (s == NULL) {
>>>      // handle failure
>>> }
>>>
>>> 2.At some places , success / failure is not handled at all .
>>>
>>> 3.Here (e.g.  LinuxDebuggerLocal.cpp)  success / failure check is done by
>>>
>>>    if (env->ExceptionOccurred()) {  ... }
>>>
>>> Which one is the best / right way to do    it (most likely not 2.) ?
>>>
>>>
>>> Best regards, Matthias
>>>
>>>
>>>
>>>>
>>>> Looks like
>>>> src/jdk.hotspot.agent/linux/native/libsaproc/LinuxDebuggerLocal.cpp
>>>> has similar issues. It would be nice to fix them as well.
>>>>
>>>> --alex
>>>>
>>>> On 02/19/2020 06:21, Baesken, Matthias wrote:
>>>>> Hello, please review this small change .
>>>>> We miss at a few places ReleaseStringUTFChars  calls in the native
>>>>> jdk.hotspot.agent  coding.
>>>>> This happens in case of  early returns .
>>>>>
>>>>>
>>>>> Bug/webrev :
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8239462
>>>>>
>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8239462.0/
>>>>>
>>>>>
>>>>> Thanks, Matthias
>>>>>