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

[David Holmes]

Hi Andrew,

This was already addressed in 8u:

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

Cheers,
David
-----

On 18/03/2020 2:49 am, Andrew Haley wrote:
> [ 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?
>