Hi Serguei and Thomas , thanks for the reviews. >Should I open a bug for these ? > Probably, two different bug are needed: hotspot/runtime and AWT.
Regarding the atoi on input provided by getenv - I’ll open 2 bugs for this. Best regards, Matthias From: serguei.spit...@oracle.com <serguei.spit...@oracle.com> Sent: Samstag, 14. September 2019 00:18 To: Baesken, Matthias <matthias.baes...@sap.com>; Thomas Stüfe <thomas.stu...@gmail.com> Cc: serviceability-dev@openjdk.java.net Subject: Re: RFR [XS]: 8230901: missing ReleaseStringUTFChars in servicability native code Hi Matthias, On 9/12/19 4:52 AM, Baesken, Matthias wrote: Hi Thomas, thanks for the review . You are correct about atoi . New webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8230901.1/<http://cr.openjdk.java.net/%7Embaesken/webrevs/8230901.1/> I had 2 additional observations : 1. With OJDK on solaris 32bit gone for quite some time, we might be able to kick out the whole non _LP64 code because we are always 64 bit (maybe someone could comment if this is a safe assumption, there might be old 32bit solaris core files flying around for some reason even these days … ) http://cr.openjdk.java.net/~mbaesken/webrevs/8230901.1/src/jdk.hotspot.agent/solaris/native/libsaproc/saproc.cpp.frames.html<http://cr.openjdk.java.net/%7Embaesken/webrevs/8230901.1/src/jdk.hotspot.agent/solaris/native/libsaproc/saproc.cpp.frames.html> 696 // some older versions of libproc.so crash when trying to attach 32 bit 697 // debugger to 64 bit core file. check and throw error. 698 #ifndef _LP64 ….. 1. The usage of atoi is commented here : https://docs.oracle.com/cd/E86824_01/html/E54766/atoi-3c.html „However, applications should not use the atoi(), atol(), or atoll() functions unless they know the value represented by the argument will be in range for the corresponding result type” ……And here : https://pubs.opengroup.org/onlinepubs/009695399/functions/atoi.html “If the number is not known to be in range, strtol()<https://pubs.opengroup.org/onlinepubs/009695399/functions/strtol.html> should be used because atoi() is not required to perform any error checking” However we have a number of usages in the coding where atoi is called without knowing that the argument is in the allowed range . some examples : src/hotspot/share/runtime/arguments.cpp-382- if (match_option(option, "-Dsun.java.launcher.pid=", &tail)) { src/hotspot/share/runtime/arguments.cpp:383: _sun_java_launcher_pid = atoi(tail); src/hotspot/share/runtime/arguments.cpp-384- continue; src/java.desktop/unix/native/libawt_xawt/xawt/XToolkit.c 455 value = getenv("_AWT_MAX_POLL_TIMEOUT"); 456 if (value != NULL) { 457 AWT_MAX_POLL_TIMEOUT = atoi(value); src/java.desktop/unix/native/common/awt/X11Color.c-781- if (getenv("CMAPSIZE") != 0) { src/java.desktop/unix/native/common/awt/X11Color.c:782: cmapsize = atoi(getenv("CMAPSIZE")); Should I open a bug for these ? Probably, two different bug are needed: hotspot/runtime and AWT. Thanks, Serguei Best regards, Matthias From: Thomas Stüfe <thomas.stu...@gmail.com><mailto:thomas.stu...@gmail.com> Sent: Donnerstag, 12. September 2019 12:22 To: Baesken, Matthias <matthias.baes...@sap.com><mailto:matthias.baes...@sap.com> Cc: serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net> Subject: Re: RFR [XS]: 8230901: missing ReleaseStringUTFChars in servicability native code Hi Matthias, your changes look good. an additional bug: http://cr.openjdk.java.net/~mbaesken/webrevs/8230901.0/src/jdk.hotspot.agent/solaris/native/libsaproc/saproc.cpp.frames.html<http://cr.openjdk.java.net/%7Embaesken/webrevs/8230901.0/src/jdk.hotspot.agent/solaris/native/libsaproc/saproc.cpp.frames.html> 698 #ifndef _LP64 699 atoi(cmdLine_cstr); 700 if (errno) { Behaviour of atoi() in error case is undefined. errno values are not defined. See: https://pubs.opengroup.org/onlinepubs/009695399/functions/atoi.html And even if atoi would set errno, this is still not enough since errno may contain a stale value. One would have to set errno=0 before the function call. If you want to fix this too 'd suggest replacing this call with strtol(). Cheers, Thomas On Thu, Sep 12, 2019 at 12:11 PM Baesken, Matthias <matthias.baes...@sap.com<mailto:matthias.baes...@sap.com>> wrote: Hello, please reviews this small change . It adds ReleaseStringUTFChars calls at some places in early return cases . ( in src/jdk.hotspot.agent/solaris/native/libsaproc/saproc.cpp THROW_NEW_DEBUGGER_EXCEPTION contains a return , see the macro declaration 39 #define THROW_NEW_DEBUGGER_EXCEPTION(str) { throwNewDebuggerException(env, str); return;} ) Bug/webrev : https://bugs.openjdk.java.net/browse/JDK-8230901 http://cr.openjdk.java.net/~mbaesken/webrevs/8230901.0/<http://cr.openjdk.java.net/%7Embaesken/webrevs/8230901.0/> Thanks, Matthias
