Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JVM default library path includes system folders #103493

Closed
fzakaria opened this issue Nov 11, 2020 · 16 comments · Fixed by #123708
Closed

JVM default library path includes system folders #103493

fzakaria opened this issue Nov 11, 2020 · 16 comments · Fixed by #123708
Labels
0.kind: bug 6.topic: java Including JDK, tooling, other languages, other VMs

Comments

@fzakaria
Copy link
Contributor

fzakaria commented Nov 11, 2020

Describe the bug
I encountered this bug when trying to use JNR that does FFI.
The particular call in general was the equivalent dlopen on the libc library and resolving to the system one which would cause a SIGSEGV.

Digging into the source some more, you discover that Java sets a default value for java.library.path if it's not explicitly set.

#ifndef OVERRIDE_LIBPATH
  #if defined(AMD64) || (defined(_LP64) && defined(SPARC)) || defined(PPC64) || defined(S390)
    #define DEFAULT_LIBPATH "/usr/lib64:/lib64:/lib:/usr/lib"
  #else
    #define DEFAULT_LIBPATH "/lib:/usr/lib"
  #endif
#else
  #define DEFAULT_LIBPATH OVERRIDE_LIBPATH
#endif

https://github.com/openjdk/jdk/blob/50357d136a775872999055bef61057b884d80693/src/hotspot/os/linux/os_linux.cpp#L413

Adding system libraries (i.e. /usr/lib) breaks/invalidates the hermetic invariant needed by Nix.
These library paths are also searched first before those set in the DT_RUNPATH of the ELF header as documented.

I suggest the OVERRIDE_LIBPATH is either set to empty string maybe or even the contents of the ELF RUNPATH header itself.

To Reproduce
I was using JRuby which makes it easy to reproduce by using a gem which ultimately calls the JNR FFI.
(The following reproduction results in the SIGSEGGV)

gem install macaddr
jruby -e "require 'macaddr'; require 'jruby/path_helper'; puts Mac.addr"

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x0000000000000680, pid=3229039, tid=0x00007ff8c5834640
#
# JRE version: OpenJDK Runtime Environment (8.0_265) (build 1.8.0_265-ga)
# Java VM: OpenJDK 64-Bit Server VM (25.265-bga mixed mode linux-amd64 compressed oops)
# Problematic frame:
# C  0x0000000000000680

Expected behavior
Expected behavior is for the FFI call to work! This can be resolved by setting java.library.path to the correct lib.

export JAVA_TOOL_OPTIONS=-Djava.library.path=/nix/store/bdf8iipzya03h2amgfncqpclf6bmy3a1-glibc-2.32/lib
❯ jruby -e "require 'macaddr'; require 'jruby/path_helper'; puts Mac.addr"
Picked up JAVA_TOOL_OPTIONS: -Djava.library.path=/nix/store/bdf8iipzya03h2amgfncqpclf6bmy3a1-glibc-2.32/lib
Picked up JAVA_TOOL_OPTIONS: -Djava.library.path=/nix/store/bdf8iipzya03h2amgfncqpclf6bmy3a1-glibc-2.32/lib
02:42:0a:fe:32:3c

NOTE: I could not merely set LD_LIBRARY_PATH because that would apply to everything in my nix-shell.
Furthermore, wrapping java is somewhat tedious, JAVA_TOOL_OPTIONS seemed pragmatic for now.

Notify maintainers
@doronbehar @roberth @manveru

Metadata

 - system: `"x86_64-linux"`
 - host os: `Linux 5.7.17-1rodete4-amd64, Debian GNU/Linux, noversion`
 - multi-user?: `no`


 - sandbox: `yes`
 - version: `nix-env (Nix) 2.3.7`
 - channels(fmzakari): `"home-manager, nixpkgs-20.09pre237891.f9eba87bf03"`
 - nixpkgs: `/home/fmzakari/.nix-defexpr/channels/nixpkgs`

Maintainer information:
All JDK derivations

@fzakaria
Copy link
Contributor Author

If you are curious for the JNR call that loads the C library: https://github.com/jnr/jnr-netdb/blob/master/src/main/java/jnr/netdb/NativeServicesDB.java#L78

            LibServices lib;
            if (os.equals(WINDOWS)) {
                Map<LibraryOption, Object> options = new HashMap<LibraryOption, Object>();
                options.put(LibraryOption.CallingConvention, CallingConvention.STDCALL);
                lib = Library.loadLibrary(LibServices.class, options, "Ws2_32");

            } else {
                String[] libnames = os.equals(SOLARIS)
                    ? new String[] { "socket", "nsl", "c" }
                    : new String[] { "c" };

                if (os.equals(LINUX)) {
                    lib = Library.loadLibrary(LinuxLibServices.class, libnames);
                } else {
                    lib = Library.loadLibrary(LibServices.class, libnames);
                }
            }

@roberth
Copy link
Member

roberth commented Nov 11, 2020

I suggest the OVERRIDE_LIBPATH is either set to empty string maybe or even the contents of the ELF RUNPATH header itself.

OVERRIDE_LIBPATH is not sufficient, because it still hardcodes SYS_EXT_DIR/lib, which is somewhere in /usr.
We'll need a patch. This also means we might as well write C code to get the right rpath value, even when re-set.
Although less useful, I'd be ok with a patch that just makes it "". At least that will give a somewhat useful error message instead of a crash.

It seems like OVERRIDE_LIBPATH was designed for cases like ours. Setting it to a static rpath value is tricky because that way we're duplicating it in a way that can't be re-set later. It must be a statically known value. So I'd go with the empty value.

@fzakaria
Copy link
Contributor Author

@roberth I'm hoping (haven't tested) that with an empty OVERRIDE_LIBPATH the DT_RUNPATH at least will be searched for.
Do you want to work on this change together ? I don't have a lot of experience yet with the JDK derivation (8.nix etc..)

@roberth
Copy link
Member

roberth commented Nov 11, 2020

@fzakaria I don't have a lot of experience with that either, but I do implement RubberDuck and I'm happy to review Nix+Java ecosystem stuff.

@fzakaria
Copy link
Contributor Author

Tagging @volth who seems to do a lot of JVM Java work as well.

@fzakaria fzakaria changed the title JVM default library path includes system folders jdk: remove default library paths included for java.library.path May 19, 2021
@fzakaria fzakaria changed the title jdk: remove default library paths included for java.library.path JVM default library path includes system folders May 19, 2021
fzakaria added a commit to fzakaria/nixpkgs that referenced this issue May 19, 2021
This patch fixes NixOS#103493 for JDK8
only.

I'm upstreaming only for JDK8 to get quorum on the approach and then
adding the patches to the remaining versions.

Improved JDK8 patch

Improved JDK8 patch
@roberth
Copy link
Member

roberth commented Jun 1, 2021

Reopening: more versions to be done

@roberth roberth reopened this Jun 1, 2021
@roberth roberth added the 6.topic: java Including JDK, tooling, other languages, other VMs label Jun 21, 2021
@stale
Copy link

stale bot commented Jan 9, 2022

I marked this as stale due to inactivity. → More info

@roberth
Copy link
Member

roberth commented Jan 11, 2022

@ghost ghost mentioned this issue Jul 27, 2022
13 tasks
@ghost
Copy link

ghost commented Jul 27, 2022

why does openjdk 17 not have a mr?

@asbachb
Copy link
Contributor

asbachb commented Jul 27, 2022

@m911t Do you have a specific problem? With the example above I could not reproduce the jvm crash.

@ghost
Copy link

ghost commented Jul 27, 2022

@asbachb sorry, i thought i was hitting this bug, but turns out that the bug i hit is different

@asbachb
Copy link
Contributor

asbachb commented Jul 27, 2022

@roberth Is there a way to reproduce this issue for openjdk17? Otherwise I'd propose to close the issue.

@roberth
Copy link
Member

roberth commented Jul 28, 2022

why does openjdk 17 not have a mr?

Afaict, it has not been written yet.

With the example above I could not reproduce the jvm crash.

You may not have the "right" libraries in /usr to make it crash.

Is there a way to reproduce this issue for openjdk17?

I don't think we need a reproducer. We know that the unpatched behavior is bad, and it is highly unlikely that upstream's behavior changes, because the rest of the world depends on it.

@fzakaria did you have a specific reason not to patch OpenJDK 17? Could you patch it?

@asbachb
Copy link
Contributor

asbachb commented Jul 28, 2022

That patch does not seem to be too complicated. I could prepare it, but I'd feel better if I had some use case to test if the patch is working.

@fzakaria
Copy link
Contributor Author

fzakaria commented Jul 28, 2022 via email

@abathur
Copy link
Member

abathur commented May 29, 2023

Closing since it looks like the last bit of this was fixed by #183760. Happy to reopen if I'm mistaken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug 6.topic: java Including JDK, tooling, other languages, other VMs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants