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

[Baesken, Matthias]

Hi Thomas,   thanks for the review .

You are correct about atoi .

New webrev  :

http://cr.openjdk.java.net/~mbaesken/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

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 ?

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

 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/

Thanks, Matthias
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190912/5d638fe7/attachment-0001.html>