RE: RFR : 8227737: avoid implicit-function-declaration on AIX

[Langer, Christoph]

Hi Matthias,



looks good, thanks for doing this.



How far are we then from enabling "warnings as errors" on AIX? :P



Best regards

Christoph



From: awt-dev  On Behalf Of Baesken, Matthias
Sent: Dienstag, 16. Juli 2019 17:04
To: Java Core Libs ; nio-...@openjdk.java.net; 
net-dev@openjdk.java.net; awt-...@openjdk.java.net
Cc: 'ppc-aix-port-...@openjdk.java.net' 
Subject: [CAUTION]  RFR : 8227737: avoid implicit-function-declaration 
on AIX



Hello, please review the following  AIX related change .

It fixes a number of  missing inclusions  leading to  
implicit-function-declaration  warnings when compiling  with the recent xlc16 
/xlclang .





At various places in the native C coding in jdk, we miss header inclusions on 
AIX.
This leads to warnings like :

/nightly/jdk/src/java.base/unix/native/libjava/childproc.c:99:5: warning: 
implicitly declaring library function 'snprintf'
with type 'int (char *, unsigned long, const char *, ...)' 
[-Wimplicit-function-declaration]
snprintf(aix_fd_dir, 32, "/proc/%d/fd", getpid());
^
/nightly/jdk/src/java.base/unix/native/libjava/childproc.c:99:5: note: include 
the header  or explicitly provide a declaration for 'snprintf'

or

/nightly/jdk/src/java.base/aix/native/libjli/java_md_aix.c:38:5: warning: 
implicitly declaring library function 'memset'
with type 'void *(void *, int, unsigned long)' [-Wimplicit-function-declaration]
memset((void *)info, 0, sizeof(Dl_info));

/nightly/jdk/src/java.base/aix/native/libjli/java_md_aix.c:38:5: note: include 
the header  or explicitly provide a declaration for 'memset'







Bug/webrev :



https://bugs.openjdk.java.net/browse/JDK-8227737



http://cr.openjdk.java.net/~mbaesken/webrevs/8227737.0/





Thanks, Matthias

RE: RFR : 8227737: avoid implicit-function-declaration on AIX

[Baesken, Matthias]

Thanks !


  *   How far are we then from enabling "warnings as errors" on AIX? :P
  *

I count  in the opt/product  jdk/jdk AIX   build  currently   60  remaining  
matches  of compiler warnings .

Plus additionally  a number of those (probably linker warnings ?) :
1500-029: (W) WARNING: subprogram ...  could not be inline

Guess we have to suppress  / ignore   at least most of the linker  inlining 
warnings   (we ignore them for a decade IMHO).


>From the  60 remaining compiler warnings   I mentioned  are  ~ 30 of type 
>Wformat :

warning: format specifies type 'int *' but the argument has type 'int' 
[-Wformat]
warning: format specifies type 'unsigned long long' but the argument has type 
'size_t' (aka 'unsigned long') [-Wformat]

Guess those can the easily fixed (and should be fixed).

Regards, Matthias


From: Langer, Christoph
Sent: Mittwoch, 17. Juli 2019 09:40
To: Baesken, Matthias ; Java Core Libs 
; nio-...@openjdk.java.net; 
net-dev@openjdk.java.net; awt-...@openjdk.java.net
Cc: 'ppc-aix-port-...@openjdk.java.net' 
Subject: RE: RFR : 8227737: avoid implicit-function-declaration on AIX

Hi Matthias,

looks good, thanks for doing this.

How far are we then from enabling "warnings as errors" on AIX? :P

Best regards
Christoph

From: awt-dev 
mailto:awt-dev-boun...@openjdk.java.net>> On 
Behalf Of Baesken, Matthias
Sent: Dienstag, 16. Juli 2019 17:04
To: Java Core Libs 
mailto:core-libs-...@openjdk.java.net>>; 
nio-...@openjdk.java.net; 
net-dev@openjdk.java.net; 
awt-...@openjdk.java.net
Cc: 'ppc-aix-port-...@openjdk.java.net' 
mailto:ppc-aix-port-...@openjdk.java.net>>
Subject: [CAUTION]  RFR : 8227737: avoid implicit-function-declaration 
on AIX

Hello, please review the following  AIX related change .
It fixes a number of  missing inclusions  leading to  
implicit-function-declaration  warnings when compiling  with the recent xlc16 
/xlclang .


At various places in the native C coding in jdk, we miss header inclusions on 
AIX.
This leads to warnings like :

/nightly/jdk/src/java.base/unix/native/libjava/childproc.c:99:5: warning: 
implicitly declaring library function 'snprintf'
with type 'int (char *, unsigned long, const char *, ...)' 
[-Wimplicit-function-declaration]
snprintf(aix_fd_dir, 32, "/proc/%d/fd", getpid());
^
/nightly/jdk/src/java.base/unix/native/libjava/childproc.c:99:5: note: include 
the header  or explicitly provide a declaration for 'snprintf'

or

/nightly/jdk/src/java.base/aix/native/libjli/java_md_aix.c:38:5: warning: 
implicitly declaring library function 'memset'
with type 'void *(void *, int, unsigned long)' [-Wimplicit-function-declaration]
memset((void *)info, 0, sizeof(Dl_info));

/nightly/jdk/src/java.base/aix/native/libjli/java_md_aix.c:38:5: note: include 
the header  or explicitly provide a declaration for 'memset'



Bug/webrev :

https://bugs.openjdk.java.net/browse/JDK-8227737

http://cr.openjdk.java.net/~mbaesken/webrevs/8227737.0/


Thanks, Matthias

Re: RFR: 8227587: Add internal privileged System.loadLibrary

[Peter Levart]

Hi Claes,

Am I reading correct that package-private 
ClassLoader.loadLibrary(Class fromClass, String name, boolean 
isAbsolute) wouldn't need to consult SecurityManager wasn't it for 
accessing system properties in private ClassLoader.initializePath(String 
propName). So perhaps if the two calls to initializePath() in the if 
block of ClassLoader.loadLibrary() are wrapped with a single 
doPrivileged(), you wouldn't have to wrap the call to 
JavaLangAccess.loadLibrary in BootLoader. And since initializePath() 
would only be called once and then the results cached, you end up with 
less doPrivileged() wrappings when SecurityManager is present as soon as 
BootLoader.loadLibrary is called the 2nd time.


BTW, there is a data race when accessing usr_paths and sys_paths static 
fields in multi-threaded scenario. Their type is String[] which means 
that the contents of the arrays could be observed uninitialized 
(elements being null) in some thread. Both fields should be made 
volatile (not only sys_paths) as assignment to them can be performed 
more than once in idempotent racy initialization.


Regards, Peter

On 7/16/19 3:48 PM, Claes Redestad wrote:

Hi,

refactored to use a BootLoader::loadLibrary API that is only visible
within the java.base module:

http://cr.openjdk.java.net/~redestad/8227587/open.03/

I've retained the bridge to ClassLoader::loadLibrary for performance,
but without any magic or privilege escalation involved. Moving sys_path
/ systemNativeLibraries out of ClassLoader seems quite complicated, so
I've not attempted that refactoring for this RFE.

/Claes

On 2019-07-12 20:03, Mandy Chung wrote:

Just to recap what we discussed offline:

We could introduce BootLoader::loadLibrary to load a system native
library for boot loader.  sys_path and systemNativeLibraries in
ClassLoader implementation are solely for boot loader and it seems
cleaner for BootLoader to handle loading of system native libraries
where it can be optimized for boot loader.  This would require
some refactoring.

One possible solution is to add a shared secret to simply call
package-private ClassLoader::loadLibrary(Class fromClass, String 
name)

method and have BootLoader::loadLibrary to check if SM is present
and wrap the call with doPriv; otherwise, just call the shared
secret loadLibrary method.

Then we can investigate in the future to refactor the native library
loading implementation to jdk.internal.loader.

what do you think?

Mandy

On 7/12/19 8:25 AM, Mandy Chung wrote:

Claes,

Thanks for exploring this.  I would like to see if we can avoid 
introducing
an internal @CS method (keep @CSM only for public APIs will help 
security

analysis).

There are other alternatives to improve the footprint performance.

One idea is java.base and java.desktop each has its own utility method
to load library.  No change is needed for java.net since only one 
loadLibrary

call.

Another idea is to add BootLoader.loadLibrary that skips the security
permission overhead and only for boot loader use.  It would require
refactoring.

I think java.base and java.desktop having its own utility method is
a simpler solution.

Mandy

On 7/12/19 7:55 AM, Claes Redestad wrote:

Hi,

I'm dropping the java.desktop changes, and Mandy has asked me to 
explore
options that does not add a new @CallerSensitive entry point, even 
to a

strongly encapsulated API like JavaLangAccess

Easiest of these is to explicitly provide the class context we're
calling from - with proper assertions. This is marginally more
performant due avoiding the Reflection.getCallerClass, but slightly 
less

convenient.

http://cr.openjdk.java.net/~redestad/8227587/open.01/

/Claes

On 2019-07-12 15:27, Roger Riggs wrote:

Hi Claes,

Looks fine.

Thanks for the updates, Roger


On 7/11/19 10:39 AM, Claes Redestad wrote:

Hi Roger,

On 2019-07-11 16:10, Roger Riggs wrote:

Hi Claes,

JavaLangAccess.java:
316: Add @param tag


done.



System.java:  2282, 2287
Runtime.loadLibrary0 makes a second check for a security manager.
Is there any potential gain by calling ClassLoader.loadLibrary 
directly?

None of the internal uses would have a separatorChar.


Most of the gain comes from not having to load one-off PA classes or
lambdas, but of course avoiding the indexOf and a few 
indirections are

marginally helpful to startup. We should at least assert that the
library names are sane, though.



I expect most of the files need a copyright update.
The script in /make/scripts/update_copyright_year.sh 
should do the work for modified files.


Fixed copyrights and updated in place: 
http://cr.openjdk.java.net/~redestad/8227587/open.00


Thanks!

/Claes

Re: RFR: 8227587: Add internal privileged System.loadLibrary

[Claes Redestad]

Hi Peter,

On 2019-07-17 10:29, Peter Levart wrote:

Hi Claes,

Am I reading correct that package-private 
ClassLoader.loadLibrary(Class fromClass, String name, boolean 
isAbsolute) wouldn't need to consult SecurityManager wasn't it for 
accessing system properties in private ClassLoader.initializePath(String 
propName). So perhaps if the two calls to initializePath() in the if 
block of ClassLoader.loadLibrary() are wrapped with a single 
doPrivileged(), you wouldn't have to wrap the call to 
JavaLangAccess.loadLibrary in BootLoader. And since initializePath() 
would only be called once and then the results cached, you end up with 
less doPrivileged() wrappings when SecurityManager is present as soon as 
BootLoader.loadLibrary is called the 2nd time.


hmm, fromClass.getClassLoader() also does a security check, though.

That said, there's a case for wrapping the initializePath calls in a
doPriv of their own, since we now have an implicit requirement that some
system library is loaded first (so that sys_paths is initialized) before
any user code attempts to load a library of their own...



BTW, there is a data race when accessing usr_paths and sys_paths static 
fields in multi-threaded scenario. Their type is String[] which means 
that the contents of the arrays could be observed uninitialized 
(elements being null) in some thread. Both fields should be made 
volatile (not only sys_paths) as assignment to them can be performed 
more than once in idempotent racy initialization.


.. and we typically seem to be loading at least one library before
initializing any user code, which is probably why this has never been an
issue. It seems like a good idea to remove this subtle fragility,
though. I wonder if we shouldn't "simply" initialize the two paths
during bootstrap and put them in final fields.

These are pre-existing issues though, so I think I'll take my reviews
and push this enhancement for now. :-)

Thanks!

/Claes



Regards, Peter

On 7/16/19 3:48 PM, Claes Redestad wrote:

Hi,

refactored to use a BootLoader::loadLibrary API that is only visible
within the java.base module:

http://cr.openjdk.java.net/~redestad/8227587/open.03/

I've retained the bridge to ClassLoader::loadLibrary for performance,
but without any magic or privilege escalation involved. Moving sys_path
/ systemNativeLibraries out of ClassLoader seems quite complicated, so
I've not attempted that refactoring for this RFE.

/Claes

On 2019-07-12 20:03, Mandy Chung wrote:

Just to recap what we discussed offline:

We could introduce BootLoader::loadLibrary to load a system native
library for boot loader.  sys_path and systemNativeLibraries in
ClassLoader implementation are solely for boot loader and it seems
cleaner for BootLoader to handle loading of system native libraries
where it can be optimized for boot loader.  This would require
some refactoring.

One possible solution is to add a shared secret to simply call
package-private ClassLoader::loadLibrary(Class fromClass, String 
name)

method and have BootLoader::loadLibrary to check if SM is present
and wrap the call with doPriv; otherwise, just call the shared
secret loadLibrary method.

Then we can investigate in the future to refactor the native library
loading implementation to jdk.internal.loader.

what do you think?

Mandy

On 7/12/19 8:25 AM, Mandy Chung wrote:

Claes,

Thanks for exploring this.  I would like to see if we can avoid 
introducing
an internal @CS method (keep @CSM only for public APIs will help 
security

analysis).

There are other alternatives to improve the footprint performance.

One idea is java.base and java.desktop each has its own utility method
to load library.  No change is needed for java.net since only one 
loadLibrary

call.

Another idea is to add BootLoader.loadLibrary that skips the security
permission overhead and only for boot loader use.  It would require
refactoring.

I think java.base and java.desktop having its own utility method is
a simpler solution.

Mandy

On 7/12/19 7:55 AM, Claes Redestad wrote:

Hi,

I'm dropping the java.desktop changes, and Mandy has asked me to 
explore
options that does not add a new @CallerSensitive entry point, even 
to a

strongly encapsulated API like JavaLangAccess

Easiest of these is to explicitly provide the class context we're
calling from - with proper assertions. This is marginally more
performant due avoiding the Reflection.getCallerClass, but slightly 
less

convenient.

http://cr.openjdk.java.net/~redestad/8227587/open.01/

/Claes

On 2019-07-12 15:27, Roger Riggs wrote:

Hi Claes,

Looks fine.

Thanks for the updates, Roger


On 7/11/19 10:39 AM, Claes Redestad wrote:

Hi Roger,

On 2019-07-11 16:10, Roger Riggs wrote:

Hi Claes,

JavaLangAccess.java:
316: Add @param tag


done.



System.java:  2282, 2287
Runtime.loadLibrary0 makes a second check for a security manager.
Is there any potential gain by calling ClassLoader.loadLibrary 
directly?

None of the internal uses would have a separatorChar.

RFR: 8227721: NetworkInterfaceRetrievalTests.java should open the java.net package

[Patrick Concannon]

Hi,

Would it be possible to have my fix for JDK-8227721 reviewed?

test/jdk/java/net/NetworkInterface/NetworkInterfaceRetrievalTests.java 
has been recently modified to perform a deep reflection ( 
setAccessible(true) ) on a non-public member 
java.net.NetworkInterface::isBoundInetAddress. However, an illegal 
access warning is generated by JTReg as a result.


This fix sets the test to run in 'othervm' mode, and gives 'open' access 
to 'java.net' to the unnamed module (where the test lives).


Further information on this fix can be found here: 
https://bugs.openjdk.java.net/browse/JDK-8227721


Webrev for fix: http://cr.openjdk.java.net/~aefimov/pconcann/8227721/00/


Kind regards,

Patrick

Re: RFR: 8227721: NetworkInterfaceRetrievalTests.java should open the java.net package

[Brian Burkhalter]

Hi Patrick,

Based on [1] this looks OK to me.

Brian

[1] https://openjdk.java.net/jtreg/tag-spec.html

> On Jul 17, 2019, at 9:52 AM, Patrick Concannon  
> wrote:
> 
> Would it be possible to have my fix for JDK-8227721 reviewed?
> 
> test/jdk/java/net/NetworkInterface/NetworkInterfaceRetrievalTests.java has 
> been recently modified to perform a deep reflection ( setAccessible(true) ) 
> on a non-public member java.net.NetworkInterface::isBoundInetAddress. 
> However, an illegal access warning is generated by JTReg as a result.
> 
> This fix sets the test to run in 'othervm' mode, and gives 'open' access to 
> 'java.net ' to the unnamed module (where the test lives).
> 
> Further information on this fix can be found here: 
> https://bugs.openjdk.java.net/browse/JDK-8227721 
> 
> 
> Webrev for fix: http://cr.openjdk.java.net/~aefimov/pconcann/8227721/00/ 
> 

Re: RFR: 8227721: NetworkInterfaceRetrievalTests.java should open the java.net package

[Chris Hegarty]

> On 17 Jul 2019, at 17:52, Patrick Concannon  
> wrote:
> 
> ...
> Webrev for fix: http://cr.openjdk.java.net/~aefimov/pconcann/8227721/00/

Looks good Patrick. Reviewed.

-Chris.

Re: RFR: 8227587: Add internal privileged System.loadLibrary

[Mandy Chung]

On 7/17/19 3:25 AM, Claes Redestad wrote:

Hi Peter,

On 2019-07-17 10:29, Peter Levart wrote:

Hi Claes,

Am I reading correct that package-private 
ClassLoader.loadLibrary(Class fromClass, String name, boolean 
isAbsolute) wouldn't need to consult SecurityManager wasn't it for 
accessing system properties in private 
ClassLoader.initializePath(String propName). So perhaps if the two 
calls to initializePath() in the if block of 
ClassLoader.loadLibrary() are wrapped with a single doPrivileged(), 
you wouldn't have to wrap the call to JavaLangAccess.loadLibrary in 
BootLoader. And since initializePath() would only be called once and 
then the results cached, you end up with less doPrivileged() 
wrappings when SecurityManager is present as soon as 
BootLoader.loadLibrary is called the 2nd time.


hmm, fromClass.getClassLoader() also does a security check, though.



Class::getClassLoader is caller-sensitive and the call to 
fromClass.getClassLoader() does not trigger any security permission 
check as it's called from NativeLibrary who is loaded by the boot loader.



That said, there's a case for wrapping the initializePath calls in a
doPriv of their own, since we now have an implicit requirement that some
system library is loaded first (so that sys_paths is initialized) before
any user code attempts to load a library of their own...



I created https://bugs.openjdk.java.net/browse/JDK-8228336 to track the 
idea to refactor the system native library loading to BootLoader.   I 
also have JDK-8228336 capturing Peter's observation.




BTW, there is a data race when accessing usr_paths and sys_paths 
static fields in multi-threaded scenario. Their type is String[] 
which means that the contents of the arrays could be observed 
uninitialized (elements being null) in some thread. Both fields 
should be made volatile (not only sys_paths) as assignment to them 
can be performed more than once in idempotent racy initialization.


.. and we typically seem to be loading at least one library before
initializing any user code, which is probably why this has never been an
issue. It seems like a good idea to remove this subtle fragility,
though. I wonder if we shouldn't "simply" initialize the two paths
during bootstrap and put them in final fields.



JDK-8228336 tracks this.

Mandy