RFD: What do we do about programs which set sys_paths to null?

[Langer, Christoph]

Hi Andrew,

to me this looks like a good and feasible solution to restore old behavior while still keeping Runtime::load and Runtime::loadLibrary free from deadlock-prone synchronization.

Can you create a JBS bug for this and set it to 11-pool?

I'll run your fix through some regression testing in 11u. There we haven't backed out JDK-8231584 yet.

Best regards
Christoph

> -----Original Message-----
> From: jdk-updates-dev <jdk-updates-dev-bounces at openjdk.java.net> On
> Behalf Of Andrew Haley
> Sent: Dienstag, 17. März 2020 17:50
> To: jdk-updates-dev at openjdk.java.net; jdk8u-dev at openjdk.java.net
> Subject: RFD: What do we do about programs which set sys_paths to null?
> 
> [ People who know all this stuff already, please feel free to skip
> this introduction. ]
> 
> This is a request for discussion about support for some programs that
> do something illegal, not at all supported. I'd like to make this work
> as intended by users, even though it's unsupported. Which seems odd,
> but I still think we should do it.
> 
> There is a long-established hack (going back about 20 years) which
> allows Java programs to set the property "java.library.path"
> dynamically. You might need to do this if you're a Java program which
> extracts a shared library from a jarfile, puts it in a tmpdir
> somewhere, then loads that library.
> 
> People discovered that even though you can set "java.library.path" in
> a program, it doesn't work as intended because the property is only
> read once at boot time, and it is then cached in the static field
> ClassLoader.usr_paths. So, people discovered that if they set
> "java.library.path" then set set ClassLoader.sys_paths to null, then
> called ClassLoader.loadLibrary, it works. Something like this:
> 
>    Class loaderClass = ClassLoader.class;
>    Field userPaths = loaderClass.getDeclaredField( "sys_paths" );
>    userPaths.setAccessible( true );
>    userPaths.set( null, null );
> 
> See the answer here, for example:
> https://community.oracle.com/thread/1551225?start=15&tstart=0
> 
> This worked because ClassLoader.loadLibrary() did this:
> 
>         if (sys_paths == null) {
>             usr_paths = initializePath("java.library.path");
>             sys_paths = initializePath("sun.boot.library.path");
>         }
> 
> The paths got re-initialized.
> 
> The back-ported fix for "8231584: Deadlock with
> ClassLoader.findLibrary and System.loadLibrary call", broke this
> hack. The code that lazily re-initializes usr_paths and sys_paths has
> gone. However, there was another important change:
> ClassLoader.loadLibrary0(), which calls ClassLoader.loadLibrary, is no
> longer synchronized, so there may now be concurrent accesses to
> ClassLoader.loadLibrary().
> 
> [ The proposed fix. ]
> 
> I'd like to add this patch to 8u and 11u:
> 
> --- a/src/java.base/share/classes/java/lang/ClassLoader.java    Mon Feb 03
> 09:39:39 2020 +0100
> +++ b/src/java.base/share/classes/java/lang/ClassLoader.java    Tue Mar 17
> 16:24:10 2020 +0000
> @@ -2562,7 +2562,7 @@
> 
>      // The paths searched for libraries
>      private static String usr_paths[];
> -    private static String sys_paths[];
> +    private static volatile String sys_paths[];
> 
>      private static String[] initializePath(String propName) {
>          String ldPath = System.getProperty(propName, "");
> @@ -2620,6 +2620,10 @@
>                              boolean isAbsolute) {
>          ClassLoader loader =
>              (fromClass == null) ? null : fromClass.getClassLoader();
> +        if (sys_paths == null) {
> +            usr_paths = initializePath("java.library.path");
> +            sys_paths = initializePath("sun.boot.library.path");
> +        }
>          assert sys_paths != null : "should be initialized at this point";
>          assert usr_paths != null : "should be initialized at this point";
> 
> This patch does nothing unless sys_paths has been set to null. There
> is some slight slowdown because a non-volatile read of sys_paths is
> now volatile, but it's IMO insignificant and in any case it's less
> than the overhead of the synchronized loadLibrary0().
> 
> I do not believe that this introduces a race when the user sets
> sys_paths to null because
> 
>    setProperty("java.library.path") happens before (write volatile sys_paths)
> (Program order)
>    (write volatile sys_paths) synchronizes with (read volatile sys_paths)
>    (read volatile sys_paths) happens before getProperty("java.lbrary.path")
> (Program order)
> 
> If the user sets sys_paths to null before setting "java.library.path"
> there is indeed a race and their program might not work, but there
> always was a race anyway.
> 
> I think we should do this for 8u and 11u. My justification is that even
> though this is an ugly hack, it satisfies a real need and we don't want
> to break users' programs in an update.
> 
> What do you think?
> 
> --
> Andrew Haley  (he/him)
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> https://keybase.io/andrewhaley
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671