RFR [XS]: 8230901: missing ReleaseStringUTFChars in servicability native code

[serguei.spitsyn at oracle.com]

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
>
> …..
>
>  2. 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.stuefe at gmail.com>
> *Sent:* Donnerstag, 12. September 2019 12:22
> *To:* Baesken, Matthias <matthias.baesken at sap.com>
> *Cc:* serviceability-dev at 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.baesken at sap.com <mailto:matthias.baesken at 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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190913/0fb3dc8a/attachment-0001.html>