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

8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries #4316

Closed
wants to merge 17 commits into from

Conversation

mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Jun 2, 2021

This patch overhauls the library loading mechanism used by the Foreign Linker API. We realized that, while handy, the default lookup abstraction (LibraryLookup::ofDefault) was behaving inconsistentlt across platforms.

This patch replaces LibraryLookup with a simpler SymbolLookup abstraction, a functional interface. Crucially, SymbolLookup does not concern with library loading, only symbol lookup. For this reason, two factories are added:

  • SymbolLookup::loaderLookup - which obtains a lookup that can be used to lookup symbols in libraries loaded by current loader
  • CLinker::systemLookup - a more stable replacement for the default lookup, which looks for symbols in libc.so (or its equivalent in other platforms). The contents of this lookup are unspecified.

Both factories are restricted, so they can only be called when --enable-native-access is set.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4316/head:pull/4316
$ git checkout pull/4316

Update a local copy of the PR:
$ git checkout pull/4316
$ git pull https://git.openjdk.java.net/jdk pull/4316/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4316

View PR using the GUI difftool:
$ git pr show -t 4316

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4316.diff

@mcimadamore
Copy link
Contributor Author

/csr

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 2, 2021

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Jun 2, 2021
@openjdk
Copy link

openjdk bot commented Jun 2, 2021

@mcimadamore this pull request will not be integrated until the CSR request JDK-8268130 for issue JDK-8268129 has been approved.

@openjdk
Copy link

openjdk bot commented Jun 2, 2021

@mcimadamore The following labels will be automatically applied to this pull request:

  • core-libs
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jun 2, 2021
@mcimadamore
Copy link
Contributor Author

mcimadamore commented Jun 2, 2021

Some notes on how the system lookup is implemented (see class SystemLookup):

  • On Linux and MacOS, we generate, at build time, an empty shim library. Since this library depends on the C standard libraries, we can, at runtime, load this shim library, and use dlsym to lookup symbols in it (since dlsym will also look at the dependencies).

  • Since Windows does not allow for library symbols in dependent libraries to be re-exported, Windows uses a different approach - which loads either ucrtbase.dll or msvcrt.dll (the former is preferred, if available), and returns a lookup object based on that.

In both cases, the required libraries are loaded in a classloader-independent fashion, by taking advantage of the support available in NativeLibraries.

Class loader lookups are built on top of ClassLoader::findNative (which is also the method used by JNI to find JNI methods).

This patch removes all the native code which was required to support the default lookup abstraction. The implementation changes are relatively straightforward - but several test and microbenchmark updates were required.

@mlbridge
Copy link

mlbridge bot commented Jun 2, 2021

Webrevs

@mlbridge
Copy link

mlbridge bot commented Jun 2, 2021

Mailing list message from Chapman Flack on security-dev:

On 06/02/21 13:30, Maurizio Cimadamore wrote:

This patch replaces `LibraryLookup` with a simpler `SymbolLookup`
abstraction, a functional interface. Crucially, `SymbolLookup` does not
concern with library loading, only symbol lookup. For this reason, two
factories are added:

Hi,

While I am thinking about this, what will be the behavior when the JVM
itself has been dynamically loaded into an embedding application, and
launched with the JNI invocation API?

Will there be a *Lookup flavor that is able to find any exported symbol
known in the embedding environment the JVM was loaded into? (This is the
sort of condition the Mac OS linker checks when given the -bundle_loader
option.)

Regards,
Chapman Flack (maintainer of a project that happens to work that way)

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Jun 2, 2021

Mailing list message from Chapman Flack on security-dev:

On 06/02/21 13:30, Maurizio Cimadamore wrote:

This patch replaces `LibraryLookup` with a simpler `SymbolLookup`
abstraction, a functional interface. Crucially, `SymbolLookup` does not
concern with library loading, only symbol lookup. For this reason, two
factories are added:

Hi,

While I am thinking about this, what will be the behavior when the JVM
itself has been dynamically loaded into an embedding application, and
launched with the JNI invocation API?

Will there be a *Lookup flavor that is able to find any exported symbol
known in the embedding environment the JVM was loaded into? (This is the
sort of condition the Mac OS linker checks when given the -bundle_loader
option.)

Regards,
Chapman Flack (maintainer of a project that happens to work that way)

Copy link
Member

@JornVernee JornVernee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor nit inline

…ort/PanamaPoint.java

Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>
@mcimadamore
Copy link
Contributor Author

Mailing list message from Chapman Flack on security-dev:

On 06/02/21 13:30, Maurizio Cimadamore wrote:

This patch replaces LibraryLookup with a simpler SymbolLookup
abstraction, a functional interface. Crucially, SymbolLookup does not
concern with library loading, only symbol lookup. For this reason, two
factories are added:

Hi,

While I am thinking about this, what will be the behavior when the JVM
itself has been dynamically loaded into an embedding application, and
launched with the JNI invocation API?

Will there be a *Lookup flavor that is able to find any exported symbol
known in the embedding environment the JVM was loaded into? (This is the
sort of condition the Mac OS linker checks when given the -bundle_loader
option.)

Regards,
Chapman Flack (maintainer of a project that happens to work that way)

Hi,
at the moment we don't have plans to add such a lookup, but I believe it should be possible to build such a lookup using dlopen and the linker API. I have provided an example elsewhere of how easy it easy to build a wrapper lookup around dlopen/dlsym:

https://gist.github.com/mcimadamore/0883ea6f4836ae0c1d2a31c48197da1a

Perhaps something like that could be useful in the use case you mention?

Copy link
Member

@PaulSandoz PaulSandoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just minor comments to accept/reject.

The shim library is an interesting approach. I did wonder if the libvm library could be used, but it might have some weird side-effects or bring in more symbols than necessary.

test/jdk/java/foreign/SafeFunctionAccessTest.java Outdated Show resolved Hide resolved
test/jdk/java/foreign/StdLibTest.java Outdated Show resolved Hide resolved
test/jdk/java/foreign/TestDowncall.java Outdated Show resolved Hide resolved
test/jdk/java/foreign/TestSymbolLookup.java Outdated Show resolved Hide resolved
test/jdk/java/foreign/TestUpcall.java Outdated Show resolved Hide resolved
test/jdk/java/foreign/TestVarArgs.java Outdated Show resolved Hide resolved
test/jdk/java/foreign/valist/VaListTest.java Outdated Show resolved Hide resolved
mcimadamore and others added 11 commits June 2, 2021 21:52
…bi/SharedUtils.java

Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Jun 2, 2021
@openjdk
Copy link

openjdk bot commented Jun 2, 2021

@mcimadamore This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries

Reviewed-by: jvernee, psandoz

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 5 new commits pushed to the master branch:

  • b955865: 8267995: Add reference to JVMS class file format in Lookup::defineHiddenClass
  • 9f05c41: 8265783: Create a separate library for x86 Intel SVML assembly intrinsics
  • e27c4d4: 8268185: Update GitHub Actions for jtreg 6
  • 68ac871: 8268189: ProblemList compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java in -Xcomp mode
  • af3df63: 8267598: jpackage removes system libraries from java.library.path

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 2, 2021
@openjdk
Copy link

openjdk bot commented Jun 3, 2021

@mcimadamore this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout symbolLookup
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Jun 3, 2021
@mcimadamore
Copy link
Contributor Author

/cc build

@openjdk openjdk bot added the build build-dev@openjdk.org label Jun 3, 2021
@openjdk
Copy link

openjdk bot commented Jun 3, 2021

@mcimadamore
The build label was successfully added.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jun 3, 2021
Copy link
Member

@erikj79 erikj79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good, just a few comments.

make/modules/jdk.incubator.foreign/Lib.gmk Show resolved Hide resolved
make/modules/jdk.incubator.foreign/Lib.gmk Outdated Show resolved Hide resolved
@mcimadamore
Copy link
Contributor Author

/integrate

@openjdk openjdk bot closed this Jun 4, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 4, 2021
@openjdk
Copy link

openjdk bot commented Jun 4, 2021

@mcimadamore Since your change was applied there have been 15 commits pushed to the master branch:

  • 40c9e25: 8265444: Javadocs: jdk.jshell - small typo
  • 069f180: 8268174: Move x86-specific stub declarations into stubRoutines_x86.hpp
  • 3025f05: 8264305: Create implementation for native accessibility peer for Statusbar java role
  • e2d5ff9: 8268214: Use system zlib and disable dtrace when building linux-aarch64 at Oracle
  • 1b4378e: 8268142: Switch to jdk-17+24 for macosx-aarch64 at Oracle
  • edca245: 8267917: mark hotspot containers tests which ignore external VM flags
  • 05df172: 8268224: Cleanup references to "strictfp" in core lib comments
  • 516e60a: 8268095: CDS MethodHandle tests should add -XX:-VerifyDependencies
  • c1f3094: 8267939: Clarify the specification of iterator and spliterator forEachRemaining
  • 460ce55: 8266019: StreamResult(File) writes to incorrect file path if # is part of the file path
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/52d8215a1ec42d67217505fe3167c70460f5a639...master

Your commit was automatically rebased without conflicts.

Pushed as commit 59a539f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@ChengJin01
Copy link

ChengJin01 commented Oct 8, 2021

Hi @mcimadamore,

As you mentioned at #4316 (comment), the system lookup is supported by the underlying NativeLibraries which also works on OpenJDK16 to support LibraryLookup::ofDefault.

But my finding is that it LibraryLookup::ofDefault works good on AIX (with libc.a) in OpenJDK16 but CLinker.systemLookup() ended up with NoSuchElementException after changing the code in SystemLookup.java and CABI.java as follows:

    private static final SymbolLookup syslookup = switch (CABI.current()) {
        case SysV, LinuxAArch64, MacOsAArch64, AIX -> libLookup(libs -> libs.loadLibrary("syslookup"));
        case Win64 -> makeWindowsLookup(); // out of line to workaround javac crash
    };

with a simple test & output:

import jdk.incubator.foreign.CLinker;
import static jdk.incubator.foreign.CLinker.*;
import jdk.incubator.foreign.SymbolLookup;
import jdk.incubator.foreign.Addressable;

public class Test {
        private static CLinker clinker = CLinker.getInstance();
        private static final SymbolLookup defaultLibLookup = CLinker.systemLookup();

        public static void main(String args[]) throws Throwable {
                Addressable strlenSymbol = defaultLibLookup.lookup("strlen").get();
        }
}

bin/java  --enable-native-access=ALL-UNNAMED  --add-modules jdk.incubator.foreign -Dforeign.restricted=permit Test
WARNING: Using incubator modules: jdk.incubator.foreign
Exception in thread "main" java.util.NoSuchElementException: No value present <-----
        at java.base/java.util.Optional.get(Optional.java:143)
        at Test.main(Test.java:11)

So I am wondering what happened to the system lookup in such case given there should be no fundamental difference in leveraging NativeLibraries (I assume the library loading in OpenJDK16 & 17 is the same at this point) unless there is something else new in OpenJDK17 I am unaware of (e.g. the changes in Lib.gmk or lib-std.m4, or a custom system lookup is required on AIX, etc). Thanks.

@mcimadamore
Copy link
Contributor Author

mcimadamore commented Oct 8, 2021

So I am wondering what happened to the system lookup in such case given there should be no fundamental difference in leveraging NativeLibraries (I assume the library loading in OpenJDK16 & 17 is the same at this point) unless there is something else new in OpenJDK17 I am unaware of (e.g. the changes in Lib.gmk or lib-std.m4, or a custom system lookup is required on AIX, etc). Thanks.

In 17, SystemLookup loads a specific library that is generated at build time - which contains all the c stdlib symbols. That's what the Lib.gmk changes are for. What I suspect is that the library is not generated at all, or not correctly on AIX, which then causes the system lookup to misbehave.

I would start by double checking that you have a file like this:

<JDK_ROOT>/lib/libsyslookup.so

And then, if the library exists, check that it has the right dependency; on my linux it's an empty library, but which depends on libc, libm and libdl:

ldd lib/libsyslookup.so  
	linux-vdso.so.1 (0x00007fff2bdf7000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f35f1def000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f35f1ca0000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f35f1c9a000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f35f2001000)

Given that you don't seem to get an error when loading syslookup.so - the most likely thing is that you have an empty library with no dynamic dependencies (given you mention libc.a - which seems a static library?). So, perhaps on AIX something similar to what we did for Windows [1] would be better.

[1] - openjdk/panama-foreign#547

@ChengJin01
Copy link

ChengJin01 commented Oct 8, 2021

Hi @mcimadamore,

Here's output of libsyslookup.so on AIX:

$ ldd  ./lib/libsyslookup.so
./lib/libsyslookup.so needs:  <--- nothing here

$ whereis libc
libc: /usr/lib/libc.a /usr/lib/libc128.a /usr/ccs/lib/libc.a

So it is high likely that there were no dependencies in this generated library.

perhaps on AIX something similar to what we did for Windows [1] would be better.

That's what I thought to be the only way around but might need to figure out the specifics on AIX.

@mcimadamore
Copy link
Contributor Author

mcimadamore commented Oct 12, 2021

That's what I thought to be the only way around but might need to figure out the specifics on AIX.

Is libc.a loadable on AIX (e.g. with System.loadLibrary) ? (Sorry I don't have a machine to test readily available). If so, we might just load libc and libm and drop the shim library generation on AIX.

@ChengJin01
Copy link

Is libc.a loadable on AIX (e.g. with System.loadLibrary) ?

I tried to load libc.a and libc this way but neither of them works on AIX.
e.g.

public class StdLibTest {
        private static CLinker clinker = CLinker.getInstance();
        static {
                System.loadLibrary("libc.a"); <-----
        }
        private static final SymbolLookup defaultLibLookup = SymbolLookup.loaderLookup();

        public static void main(String args[]) throws Throwable {
                Addressable strlenSymbol = defaultLibLookup.lookup("strlen").get();
        }
}

$ ./bin/java  --enable-native-access=ALL-UNNAMED  --add-modules jdk.incubator.foreign 
-Dforeign.restricted=permit --enable-native-access=ALL-UNNAMED  StdLibTest
WARNING: Using incubator modules: jdk.incubator.foreign
Exception in thread "main" java.lang.UnsatisfiedLinkError: Can't load libc.a <-------
        at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:1822)
        at java.base/java.lang.System.loadLibrary(System.java:694)
        at StdLibTest.<clinit>(StdLibTest.java:23)

and

public class StdLibTest {
        private static CLinker clinker = CLinker.getInstance();
        static {
                System.loadLibrary("libc"); <-------
        }
        private static final SymbolLookup defaultLibLookup = SymbolLookup.loaderLookup();

        public static void main(String args[]) throws Throwable {
                Addressable strlenSymbol = defaultLibLookup.lookup("strlen").get();
        }
}

$ ./bin/java  --enable-native-access=ALL-UNNAMED  --add-modules jdk.incubator.foreign
-Dforeign.restricted=permit --enable-native-access=ALL-UNNAMED  StdLibTest
WARNING: Using incubator modules: jdk.incubator.foreign
Exception in thread "main" java.lang.UnsatisfiedLinkError: Can't load libc <------
        at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:1822)
        at java.base/java.lang.System.loadLibrary(System.java:694)
        at StdLibTest.<clinit>(StdLibTest.java:23)

@mcimadamore
Copy link
Contributor Author

I tried to load libc.a and libc this way but neither of them works on AIX.

Sorry - what I meant is - System::load, which accepts a full path and extension. E.g.

System.load("/usr/lib/libc.a");

@ChengJin01
Copy link

Just tried with System.load() but still ended up with pretty much the same failure given both of them eventually invokes ClassLoader.loadLibrary to load the library in which case there is no big difference at this point.

        static {
                System.load("/usr/lib/libc.a");
        }

Exception in thread "main" java.lang.UnsatisfiedLinkError: Can't load /usr/lib/libc.a <----
        at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:1793)
        at java.base/java.lang.System.load(System.java:672)
        at StdLibTest.<clinit>(StdLibTest.java:24)

@mcimadamore
Copy link
Contributor Author

Just tried with System.load() but still ended up with pretty much the same failure given both of them eventually invokes ClassLoader.loadLibrary to load the library in which case there is no big difference at this point.

Yes and no. System::loadLibrary wants a library name (no extension). It will add a library prefix (e.g. lib on linux) and a library suffix (e.g. .so on linux). So, if you do:

System.loadLibrary("c")

You will end up with libc.so. The System::loadLibrary logic will then try to find that file in any of the known library paths.

System.load avoids this by accepting the full path of the library. So there's no guessing the path, nor guessing of prefix/suffix. But it seems like loading still fails, likely because we try to load this library with dlopen but this is a static library, so for dlopen it just doesn't make sense.

@ChengJin01
Copy link

If so, I am wondering whether there is anything else left (inherited from JDK16) in JDK17 we can leverage to support libc.a on AIX except the way similar to Windows.

@JornVernee
Copy link
Member

JornVernee commented Oct 12, 2021

I think the reason it worked in JDK 16 is because all the symbols in the JVM were visible through the system lookup, and the JVM statically links the standard library (AFAIU). So, just because the VM code depended on something, it could be loaded by the system lookup. But, that would mean that not all symbols in the standard library were visible... and also, being able to find any symbol in the JVM was a bug.

I think the only solution here - assuming that libc is not available as a dynamic library on typical AIX systems - is to figure out how to repackage these static libraries as a dynamic library, retaining all the symbols, and then bundle that dynamic library with the JDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated security security-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

5 participants