8286978: SIGBUS in libz during CDS initialization #8799

calvinccheung · 2022-05-20T01:05:18Z

Please review this fix to ensure all requested bytes are transferred by calling FileChannel.transferFrom in a loop and checking its return value.

Tested by running the SharedArchiveConsistency.java test on linux-aarch64 30 times on each of the following tier3 options:

-XX:+CreateCoredumpOnCrash -XX:+UseSerialGC
-XX:+CreateCoredumpOnCrash -XX:+UseParallelGC -XX:+UseNUMA

Progress

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

Issue

JDK-8286978: SIGBUS in libz during CDS initialization

Reviewers

Ioi Lam (@iklam - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8799

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

Using diff file

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

bridgekeeper · 2022-05-20T01:05:32Z

👋 Welcome back ccheung! 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.

calvinccheung · 2022-05-20T01:05:42Z

/label add hotspot-runtime

openjdk · 2022-05-20T01:07:38Z

@calvinccheung
The hotspot-runtime label was successfully added.

mlbridge · 2022-05-20T01:55:14Z

Webrevs

…f code

iklam · 2022-05-20T06:24:04Z

test/lib/jdk/test/lib/cds/CDSArchiveUtils.java

            inputChannel.position(offset + nBytes);
-            outputChannel.transferFrom(inputChannel, offset, orgSize - nBytes);
+            long length = orgSize - nBytes;


Line 458 doesn't seem to be needed.

The line was removed in version v1 https://openjdk.github.io/cr/?repo=jdk&pr=8799&range=01#sdiff-0-test/lib/jdk/test/lib/cds/CDSArchiveUtils.java

iklam · 2022-05-20T06:24:04Z

test/lib/jdk/test/lib/cds/CDSArchiveUtils.java

    // dstFile will keep original size so will remove corresponding bytes.length bytes at end of file
    public static File insertBytesRandomlyAfterHeader(File orgFile, String newFileName, byte[] bytes) throws Exception {
        long offset = fileHeaderSize(orgFile) + getRandomBetween(0L, 4096L);
        File dstFile = new File(newFileName);
        try (FileChannel inputChannel = new FileInputStream(orgFile).getChannel();
             FileChannel outputChannel = new FileOutputStream(dstFile).getChannel()) {
            long orgSize = inputChannel.size();
-            outputChannel.transferFrom(inputChannel, 0, offset);
+            transferFrom(inputChannel, outputChannel, 0, offset);
            outputChannel.position(offset);
            outputChannel.write(ByteBuffer.wrap(bytes));


The changes for transferFrom looks good, but we still have a problem with outputChannel.write(), which can also return fewer bytes than requested (or even zero).

For simplicity, I think it's best to ditch FileChannel and use FileOutputStream.write() instead.

Another option is to avoid using outputChannel.write(). Instead, duplicate some bytes from the end of the header.

public static File insertBytesRandomlyAfterHeader(File orgFile, String newFileName) throws Exception { long headerSize = fileHeaderSize(orgFile); long dupSize = getRandomBetween(0L, headerSize); File dstFile = new File(newFileName); try (FileChannel inputChannel = new FileInputStream(orgFile).getChannel(); FileChannel outputChannel = new FileOutputStream(dstFile).getChannel()) { long orgSize = inputChannel.size(); // Copy the header transferFrom(inputChannel, outputChannel, 0, headerSize); // Copy dupSize bytes from the end of the header. Then, copy the rest // of the input such that the new file will have the same size as // the old file. inputChannel.position(headerSize - dupSize); transferFrom(inputChannel, outputChannel, headerSize, orgSize - headerSize); } return dstFile; }

Thanks for the suggestions. I've pushed another commit with the above changes. I also changed the callsite in SharedArchiveConsistency.java since there's no need to pass in a byte array.

iklam

LGTM

openjdk · 2022-05-20T17:30:59Z

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

8286978: SIGBUS in libz during CDS initialization

Reviewed-by: iklam

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

4587337: 8286858: Remove dead code in sun.reflect.misc.MethodUtil
6d56caf: 8285962: NimbusDefaults has a typo in a L&F property
d5d19f5: 8287013: StringConcatFactory: remove short and byte mixers/prependers
828dc89: 8286893: G1: Recent card set coarsening statistics wrong
6569666: 8286943: G1: With virtualized remembered sets, maximum number of cards configured is wrong
40e99a1: 8285308: Win: Japanese logical fonts are drawn with wrong size
890771e: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character
de74e0e: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable
9f562ef: 8286872: Refactor add/modify notification icon (TrayIcon)
b089229: 8271078: jdk/incubator/vector/Float128VectorTests.java failed a subtest
... and 6 more: https://git.openjdk.java.net/jdk/compare/83cec4b6cc25b9b0db751bbe873444259cdcb21c...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.

calvinccheung · 2022-05-20T17:51:01Z

Thanks @iklam for the review.
/integrate

openjdk · 2022-05-20T17:51:58Z

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

4587337: 8286858: Remove dead code in sun.reflect.misc.MethodUtil
6d56caf: 8285962: NimbusDefaults has a typo in a L&F property
d5d19f5: 8287013: StringConcatFactory: remove short and byte mixers/prependers
828dc89: 8286893: G1: Recent card set coarsening statistics wrong
6569666: 8286943: G1: With virtualized remembered sets, maximum number of cards configured is wrong
40e99a1: 8285308: Win: Japanese logical fonts are drawn with wrong size
890771e: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character
de74e0e: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable
9f562ef: 8286872: Refactor add/modify notification icon (TrayIcon)
b089229: 8271078: jdk/incubator/vector/Float128VectorTests.java failed a subtest
... and 6 more: https://git.openjdk.java.net/jdk/compare/83cec4b6cc25b9b0db751bbe873444259cdcb21c...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-05-20T17:52:13Z

@calvinccheung Pushed as commit 087bccf.

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

8286978: SIGBUS in libz during CDS initialization

a78672c

openjdk bot added the hotspot-runtime label May 20, 2022

calvinccheung marked this pull request as ready for review May 20, 2022

openjdk bot added the rfr label May 20, 2022

corrected the condition of the while loop, removed an unneeded line o…

9147888

…f code

iklam suggested changes May 20, 2022

View changes

avoid the use of FileChannel.write

752e64a

iklam approved these changes May 20, 2022

View changes

openjdk bot added the ready label May 20, 2022

openjdk bot added the integrated label May 20, 2022

openjdk bot closed this May 20, 2022

openjdk bot removed ready rfr labels May 20, 2022

calvinccheung deleted the CDSArchiveUtils-transferFrom branch May 20, 2022

8286978: SIGBUS in libz during CDS initialization #8799

8286978: SIGBUS in libz during CDS initialization #8799

calvinccheung commented May 20, 2022 •

edited by openjdk[bot] bot

bridgekeeper bot commented May 20, 2022

calvinccheung commented May 20, 2022

openjdk bot commented May 20, 2022

mlbridge bot commented May 20, 2022 •

edited

iklam May 20, 2022

calvinccheung May 20, 2022

iklam May 20, 2022

iklam May 20, 2022

calvinccheung May 20, 2022

iklam left a comment

openjdk bot commented May 20, 2022 •

edited

calvinccheung commented May 20, 2022

openjdk bot commented May 20, 2022

openjdk bot commented May 20, 2022

8286978: SIGBUS in libz during CDS initialization #8799

8286978: SIGBUS in libz during CDS initialization #8799

Conversation

calvinccheung commented May 20, 2022 • edited by openjdk[bot] bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 20, 2022

calvinccheung commented May 20, 2022

openjdk bot commented May 20, 2022

mlbridge bot commented May 20, 2022 • edited

Webrevs

iklam May 20, 2022

Choose a reason for hiding this comment

calvinccheung May 20, 2022

Choose a reason for hiding this comment

iklam May 20, 2022

Choose a reason for hiding this comment

iklam May 20, 2022

Choose a reason for hiding this comment

calvinccheung May 20, 2022

Choose a reason for hiding this comment

iklam left a comment

openjdk bot commented May 20, 2022 • edited

calvinccheung commented May 20, 2022

openjdk bot commented May 20, 2022

openjdk bot commented May 20, 2022

calvinccheung commented May 20, 2022 •

edited by openjdk[bot] bot

mlbridge bot commented May 20, 2022 •

edited

openjdk bot commented May 20, 2022 •

edited