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