8333144: docker tests do not work when ubsan is configured

[MBaesken]

Currently when we run with ubsan - enabled binaries (configure option --enable-ubsan, see JDK-8298448), the docker tests do not work.

We find this in the test output

[STDOUT]
/jdk/bin/java: error while loading shared libraries: libubsan.so.1: cannot open shared object file: No such file or directory

The container where the test is executed does not contain the ubsan package; we might skip the test in this case.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Warning

 ⚠️ Found leading lowercase letter in issue title for 8333144: docker tests do not work when ubsan is configured

Issue

• JDK-8333144: docker tests do not work when ubsan is configured (Bug - P4)

Reviewers

• Christoph Langer (@RealCLanger - Reviewer)
• Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19907/head:pull/19907
$ git checkout pull/19907

Update a local copy of the PR:
$ git checkout pull/19907
$ git pull https://git.openjdk.org/jdk.git pull/19907/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19907

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19907.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mbaesken! 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]

@MBaesken 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:

8333144: docker tests do not work when ubsan is configured

Reviewed-by: clanger, stuefe

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 111 new commits pushed to the master branch:

• 066db6e: 8337274: Remove repeated 'the' in StyleSheet.create{Small,Large}AttributeSet
• 328a053: 8335059: Consider renaming ClassLoaderData::keep_alive
• a89b525: 8337721: G1: Remove unused G1CollectedHeap::young_collection_verify_type
• dc35f3e: 8337062: x86_64: Unordered add/mul reduction support for vector api
• 21e86d1: 8334405: java/nio/channels/Selector/SelectWithConsumer.java#id0 failed in testWakeupDuringSelect
• f1fa64b: 4966250: SSLSessionContext.setSessionTimeout() documentation could be updated
• 9fe6e23: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception
• 27af80e: 8324260: java/foreign/TestStubAllocFailure.java run timeout with -Xcomp
• 022899a: 8337641: G1: Remove unused G1CollectedHeap::alloc_highest_free_region
• c6f0a35: 8333317: Test sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java failed with: Invalid ECDH ServerKeyExchange signature
• ... and 101 more: https://git.openjdk.org/jdk/compare/4c7b3e7fc39a06e208ea1668095d055457549d63...master

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]

@MBaesken The following label will be automatically applied to this pull request:

• hotspot

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

[mlbridge]

Webrevs

• 03: Full - Incremental (c4c3fdbe)
• 02: Full - Incremental (4a792430)
• 01: Full - Incremental (ba4f63be)
• 00: Full (35163ff7)

[bridgekeeper]

@MBaesken This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[MBaesken]

Hello, any input on this ?
Currently the situation with the jtreg tests is not good when running ubsan-enabled binaries .

So we have to check for ubsan (see the PR for an example) or install ubsan into the docker (or podman or ...) container .

[dholmes-ora]

On the one hand this seems like a "Dr Dr it hurts when I do this" kind of problem. On the other hand it only affects the docker testing so I'm inclined to let it in, even though it is a bit of a blunt instrument (what if ubsan is installed in the container and someone wants to run with it enabled there?).

[MBaesken]

what if ubsan is installed in the container and someone wants to run with it enabled there

We could also try to install the ubsan package into the test container, at least for the default container setup.
Do you prefer that ?

[dholmes-ora]

I think others who have more investment in this area need to weigh in. I don't know the implications for our infra folk if we need to ensure ubsan is installed.

[MBaesken]

I think others who have more investment in this area need to weigh in. I don't know the implications for our infra folk if we need to ensure ubsan is installed.

I think this would be in the standard container config / BUILDFILE we use for the tests. So if all works well, no implications.

On the other hand, we could also just run the ubsan - based tests with an own exclude/problem list and exclude the docker test that currently cannot work. That needs a separate list but no other src changes like this PR or the idea with adjusted docker container config.

[RealCLanger]

I would prefer to add the required ubsan libraries to the container used for testing when testing an ubsan enabled build. Can we achieve this?

[MBaesken]

I would prefer to add the required ubsan libraries to the container used for testing when testing an ubsan enabled build. Can we achieve this?

I added libubsan1 to the container (tested it and works nicely, should do no harm if we test a non-ubsan build).
Should we go this way ?
If so I could remove the WhiteBox related changes (or keep it for other usages).
I also tried to add a WhiteBox based check to 'DockerTestUtils.java' but this seems not to work. But as i said it is probably not necessary.

[RealCLanger]

I think adding libubsan1 to the test container is the best way to go. If it cannot be made conditional on ubsan builds then be it so. Then the Whitebox changes should be removed obviously.

[MBaesken]

I removed the WhiteBox stuff.
Maybe David could give the change a try in the Oracle CI if that's possible ?

[RealCLanger]

Fine for me.

[tstuefe]

I have seen this too late.. some questions:

• Are we sure the images the tests use will always be Debian or Debian descendants? What about RHEL or Oracle Linux?
• How close does the libubsan have to be to the compiler used to build the tested JVM? After all, UBSAN is a compiler feature. Does this work for any version of GCC used to build the JVM? Or do we risk weird test errors if libubsan in the image is incompatible with the GCC version used when building the testee JVM?

[MBaesken]

Are we sure the images the tests use will always be Debian or Debian descendants? What about RHEL or Oracle Linux?

We use Ubuntu for the container test base image as default , see

jdk/test/lib/jdk/test/lib/containers/docker/DockerfileConfig.java

Line 47 in 65646b5

switch (Platform.getOsArch()) {

To be more on the safe side (potentially the image can be switched with jdk.test.docker.image.name) we could add a check (and avoid adding the libubsan1 package if it is not ubuntu).

Regarding compatibility, I've seen no issues (and if you compile without ubsan you would not reference the libubsan1 anyway). So it is for some special configuration.

[dholmes-ora]

Maybe David could give the change a try in the Oracle CI if that's possible ?

Testing ...

[dholmes-ora]

This had no impact on our tier5 testing where we test container related stuff.

[MBaesken]

Hi David, thanks for testing.
Thinking more about it, should I test in generateDockerFile for baseImage name containing ubuntu, so that people using a system property to set an own special image do not run into issues with the added RUN apt-get install libubsan1 ?

[tstuefe]

Are we sure the images the tests use will always be Debian or Debian descendants? What about RHEL or Oracle Linux?

We use Ubuntu for the container test base image as default , see

jdk/test/lib/jdk/test/lib/containers/docker/DockerfileConfig.java

Line 47 in 65646b5

switch (Platform.getOsArch()) {

To be more on the safe side (potentially the image can be switched with jdk.test.docker.image.name) we could add a check (and avoid adding the libubsan1 package if it is not ubuntu).

Regarding compatibility, I've seen no issues (and if you compile without ubsan you would not reference the libubsan1 anyway). So it is for some special configuration.

Okay, thanks Matthias

[dholmes-ora]

@MBaesken I know nothing about container setup, sorry.

[tstuefe]

Looks reasonable

[MBaesken]

Hi Thomas and Christoph, thanks for the reviews !
If the added apt-get call causes issues for some people running the tests, we can easily add an ubuntu distro check in a follow up.

[MBaesken]

/integrate

[openjdk]

Going to push as commit fbe8a81.
Since your change was applied there have been 117 commits pushed to the master branch:

• dabdae6: 8334599: Improve code from JDK-8302671
• 8bd3cd5: 8337716: ByteBuffer hashCode implementations are inconsistent
• 367e0a6: 8334755: Asymptotically faster implementation of square root algorithm
• 34edc73: 8337396: Cleanup usage of ExternalAddess
• 7deee74: 8335122: Reorganize internal low-level support for HTML in jdk.javadoc
• f2e1205: 8337676: JFR: Change the label of the throttle setting
• 066db6e: 8337274: Remove repeated 'the' in StyleSheet.create{Small,Large}AttributeSet
• 328a053: 8335059: Consider renaming ClassLoaderData::keep_alive
• a89b525: 8337721: G1: Remove unused G1CollectedHeap::young_collection_verify_type
• dc35f3e: 8337062: x86_64: Unordered add/mul reduction support for vector api
• ... and 107 more: https://git.openjdk.org/jdk/compare/4c7b3e7fc39a06e208ea1668095d055457549d63...master

Your commit was automatically rebased without conflicts.

[openjdk]

@MBaesken Pushed as commit fbe8a81.

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

[jerboaa]

If the added apt-get call causes issues for some people running the tests, we can easily add an ubuntu distro check in a follow up.

@MBaesken This breaks container tests on non Debian distros. Please add some form of property that needs to be set to install libubsan1 on the test containers if you need that. It should have no impact on users not needing/using it. Thanks!

[tstuefe]

If the added apt-get call causes issues for some people running the tests, we can easily add an ubuntu distro check in a follow up.

@MBaesken This breaks container tests on non Debian distros. Please add some form of property that needs to be set to install libubsan1 on the test containers if you need that. It should have no impact on users not needing/using it. Thanks!

weird that Oracle reported no errors since I assume they test on what essentially is RHEL

[jerboaa]

If the added apt-get call causes issues for some people running the tests, we can easily add an ubuntu distro check in a follow up.

@MBaesken This breaks container tests on non Debian distros. Please add some form of property that needs to be set to install libubsan1 on the test containers if you need that. It should have no impact on users not needing/using it. Thanks!

weird that Oracle reported no errors since I assume they test on what essentially is RHEL

It would depend whether or not skipped tests are flagged or not. The container tests get silently skipped if the container build of the image to test fails. That might be a reason.

[MBaesken]

I created
https://bugs.openjdk.org/browse/JDK-8338550
8338550: Make libubsan1 installation in test container optional

[jerboaa]

Thanks!