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

[Baesken, Matthias]

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

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