8191278: MappedByteBuffer bulk access memory failures are not handled gracefully

[snazarkin]

Hi!
I'd like to backport this changes to fix crash happens on memory mapped file operations. The patch applies almost cleanly except stubGenerator_arm.cpp due to miss of JEP 340 (One AArch64 Port, Not Two), and some copyright year conflicts.
Need to note that a series of follow up is required to fix some platforms build. Particularly arm32/aarch64 is not buildable without 8226871. I'll create necessary PR soon.
Tested with tier1/tier2 and new InternalErrorTest.

---

Progress

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

Issue

• JDK-8191278: MappedByteBuffer bulk access memory failures are not handled gracefully

Reviewers

• @eastig (no known github.com user name / role)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/173/head:pull/173
$ git checkout pull/173

Update a local copy of the PR:
$ git checkout pull/173
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/173/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 173

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/173.diff

[bridgekeeper]

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

This backport pull request has now been updated with issue and summary from the original commit.

[mlbridge]

Webrevs

• 00: Full (745114d5)

[eastig]

LGTM

Confirm changes to files except stubGenerator_arm.cpp are identical to the original patch. Changes to stubGenerator_arm.cpp look good to me.

[eastig]

Particularly arm32/aarch64 is not buildable without 8226871. I'll create necessary PR soon.

BTW, 8226871 looks trivial. Maybe it is worth to push it first.

[RealCLanger]

Hi, @snazarkin,
since JDK-8191278 has a few follow-up fixes, could you please use the recently enabled feature of dependent pull requests and add all those. We can then test them together.
To do that, create the backport of e.g. JDK-8226871 on top of this branch and then open a PR to branch pr/173 of this repository.
Please let me know if you have further questions.
Thanks!

[snazarkin]

To do that, create the backport of e.g. JDK-8226871 on top of this branch and then open a PR to branch pr/173 of this repository.

Thanks for information. I've added PRs with changes made independently from code introduced by this PR. Hovewer it is not clear how to add fix for this PR covered by 8229254.

[snazarkin]

Particularly arm32/aarch64 is not buildable without 8226871. I'll create necessary PR soon.
BTW, 8226871 looks trivial. Maybe it is worth to push it first.

Thanks. Added depended pull request.

[snazarkin]

LGTM

Confirm changes to files except stubGenerator_arm.cpp are identical to the original patch. Changes to stubGenerator_arm.cpp look good to me.

Thanks for review. The review would be simpler if diff viewer can omit while space changes.

[eastig]

Thanks for review. The review would be simpler if diff viewer can omit while space changes.

It can. In 'File changed' tab you can click the gear and select 'Hide whitespace changes'.

[RealCLanger]

To do that, create the backport of e.g. JDK-8226871 on top of this branch and then open a PR to branch pr/173 of this repository.

Thanks for information. I've added PRs with changes made independently from code introduced by this PR. Hovewer it is not clear how to add fix for this PR covered by 8229254.

You should be able to do 8229254 as dependend PR to this PR. Just cherry pick the change on a branch based on snazarkin:8226878-backport (or pr/173) and then open the PR against pr/173. That should be exactly what dependend PRs are designed for.

[snazarkin]

I've complete all 3 follow-ups to cover issue introduced by this PR

[RealCLanger]

I've complete all 3 follow-ups to cover issue introduced by this PR

Thanks, @snazarkin. I'll run the PRs through SAP's regression testing and reach out to the fellow maintainers for some alignment on whether this backport is appropriate for 11u at this time. Judging by the lines of code that are changed by this - and most of these are at the very heart of hotspot - we have to be very careful with this one. Thanks for your understanding.

[mlbridge]

Mailing list message from Andrew Haley on jdk-updates-dev:

On 7/29/21 1:28 PM, Sergey Nazarkin wrote:

I'd like to backport this changes to fix crash happens on memory mapped file operations. The patch applies almost cleanly except stubGenerator_arm.cpp due to miss of JEP 340 (One AArch64 Port, Not Two), and some copyright year conflicts.
Need to note that a series of follow up is required to fix some platforms build. Particularly arm32/aarch64 is not buildable without 8226871. I'll create necessary PR soon.
Tested with tier1/tier2 and new InternalErrorTest.

Wow. This is a very invasive patch for a P3 enhancement (that mysteriously
got upgraded to a P2 for backports.) We'd need to see evidence of significant
hardship to users in order to justify approval.

As far as I know, this only has any effect if someone modifies a file in
some way while a mapping is open.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

[mlbridge]

Mailing list message from Sergey Nazarkin on jdk-updates-dev:

The patch looks scary but in general it just adds a guard at the beginning of unsafe copy block. So a bunch of code just shifted right without single line modification.

We got a few reports from the users about JVM crash when they modify file being mapped. It was definitely application issue but Unsafe contains protection for primitive types so it might be worth to have the same for copy operation.

On Aug 5, 2021, at 11:46, Andrew Haley <aph at redhat.com> wrote:

On 7/29/21 1:28 PM, Sergey Nazarkin wrote:

I'd like to backport this changes to fix crash happens on memory mapped file operations. The patch applies almost cleanly except stubGenerator_arm.cpp due to miss of JEP 340 (One AArch64 Port, Not Two), and some copyright year conflicts.
Need to note that a series of follow up is required to fix some platforms build. Particularly arm32/aarch64 is not buildable without 8226871. I'll create necessary PR soon.
Tested with tier1/tier2 and new InternalErrorTest.

Wow. This is a very invasive patch for a P3 enhancement (that mysteriously
got upgraded to a P2 for backports.) We'd need to see evidence of significant
hardship to users in order to justify approval.

As far as I know, this only has any effect if someone modifies a file in
some way while a mapping is open.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

[mlbridge]

Mailing list message from Andrew Haley on jdk-updates-dev:

On 8/5/21 8:15 PM, Sergey Nazarkin wrote:

The patch looks scary but in general it just adds a guard at the beginning of unsafe copy block. So a bunch of code just shifted right without single line modification.

We got a few reports from the users about JVM crash when they modify file being mapped. It was definitely application issue but Unsafe contains protection for primitive types so it might be worth to have the same for copy operation.

Sure, I understand. However, the only thing this patch changes is that
you get a Java stack trace instead of a VM dump. Your program exits,
either way. So it really is just about being graceful.

The default response to enhancements in old releases is no, and there's
a good reason for that.

We need to learn from experience. Take, for example, JDK-8267689. That
was a fix for a rare and hard-to-provoke crash when accessing
misaligned Unsafe memory. The backport patch looked reasonable enough,
and was working fine on the head release, but it was wrong. In fact,
the head release had a subtle but latent bug, which wasn't so latent
in 8u. This bug was worse than the original bug that JDK-8267689 was
supposed to fix.

I think we shouldn't risk regressions like that unless we actually
need to.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

[mlbridge]

Mailing list message from Sergey Nazarkin on jdk-updates-dev:

Thanks, Andrew! That sounds like a reason to not bother the community with the patch for jdk8.

On Aug 6, 2021, at 12:33, Andrew Haley <aph-open at littlepinkcloud.com> wrote:

On 8/5/21 8:15 PM, Sergey Nazarkin wrote:

The patch looks scary but in general it just adds a guard at the beginning of unsafe copy block. So a bunch of code just shifted right without single line modification.

We got a few reports from the users about JVM crash when they modify file being mapped. It was definitely application issue but Unsafe contains protection for primitive types so it might be worth to have the same for copy operation.

Sure, I understand. However, the only thing this patch changes is that
you get a Java stack trace instead of a VM dump. Your program exits,
either way. So it really is just about being graceful.

The default response to enhancements in old releases is no, and there's
a good reason for that.

We need to learn from experience. Take, for example, JDK-8267689. That
was a fix for a rare and hard-to-provoke crash when accessing
misaligned Unsafe memory. The backport patch looked reasonable enough,
and was working fine on the head release, but it was wrong. In fact,
the head release had a subtle but latent bug, which wasn't so latent
in 8u. This bug was worse than the original bug that JDK-8267689 was
supposed to fix.

I think we shouldn't risk regressions like that unless we actually
need to.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

[RealCLanger]

FWIW: Testing of this change plus its three follow ups didn't show regressions in SAP's nightlies.

[RealCLanger]

@snazarkin, since this item was rejected by the maintainers, would you mind closing this PR?