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

[serguei . spitsyn]

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.stu...@gmail.com>
*Sent:* Donnerstag, 12. September 2019 12:22
*To:* Baesken, Matthias <matthias.baes...@sap.com>
*Cc:* 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