JDK-8295889: NMT preinit code does not handle allocation errors #10855

tstuefe · 2022-10-25T15:18:05Z

The NMT preinit allocator (used for os::malloc and friends before the VM is initialized) does not handle malloc errors, nor does it handle overflows due to large sizes (it uses malloc headers too). Both cases need to be handled.

However, we can keep matters very simple. No need to propagate errors up to the caller; we can just fatal out on errors here since, in this phase, there is no alternative for failed allocations.

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

Issue

JDK-8295889: NMT preinit code does not handle allocation errors

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Matthias Baesken (@MBaesken - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10855

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

Using diff file

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

bridgekeeper · 2022-10-25T15:18:19Z

👋 Welcome back stuefe! 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 · 2022-10-25T15:20:57Z

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

hotspot-runtime

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.

tstuefe · 2022-10-25T16:17:07Z

Linux x86 errors unrelated. I did build and test manually 32-bit on Ubuntu, all fine.

mlbridge · 2022-10-25T16:24:26Z

Webrevs

01: Full - Incremental (ac9c3365)
00: Full (76d9d5ba)

dholmes-ora

Doesn't seem unreasonable to check for OOM even though highly unlikely during VM init. But how would an overflow arise in practice? Seems like something more suited for an assertion.

tstuefe · 2022-10-26T05:35:21Z

Doesn't seem unreasonable to check for OOM even though highly unlikely during VM init. But how would an overflow arise in practice? Seems like something more suited for an assertion.

Hi David,

thanks for looking at this. Assert makes sense, I will do that. It can happen only by malpractice, if size is controlled via outside input. I cannot see a path where this would happen in VM preinit time though, so this is just theoretical.

The issue was inspired by 8286519, where Harold did the same os::malloc. That made me look more closely at some corner cases and how they are handled. Another issue I work on is https://bugs.openjdk.org/browse/JDK-8295865.

dholmes-ora · 2022-10-26T05:52:23Z

Overflow is really only a concern if the incoming value could be user-supplied/influenced. For pure VM code an assert would suffice to guard against accident (mainly via whitebox or other test code that might try to hit edge cases).

dholmes-ora · 2022-10-26T05:54:23Z

src/hotspot/share/services/nmtPreInit.cpp

 // --------- NMTPreInitAllocation --------------

 NMTPreInitAllocation* NMTPreInitAllocation::do_alloc(size_t payload_size) {
  const size_t outer_size = sizeof(NMTPreInitAllocation) + payload_size;
+  guarantee(outer_size > payload_size, "Overflow");


I was suggesting an assertion as this is a VM programming error we should catch during testing in't it?

Guarantee as a protection against future bitrot. I am not even 100% sure there are not paths today that are controlled via outside input. I am even less sure about the future. Therefore I protect the release version too. Are you concerned about runtime costs? It is just a comparison.

OTOH I have no strong emotions here. If you insist, I can make it debug only.

I'm more concerned that we don't know if this path may be influenced by external input. :(

These kind of "just in case" checks all add up.

But I won't insist on a change.

dholmes-ora · 2022-10-26T07:32:02Z

src/hotspot/share/services/nmtPreInit.cpp

 // --------- NMTPreInitAllocation --------------

 NMTPreInitAllocation* NMTPreInitAllocation::do_alloc(size_t payload_size) {
  const size_t outer_size = sizeof(NMTPreInitAllocation) + payload_size;
+  guarantee(outer_size > payload_size, "Overflow");


I'm more concerned that we don't know if this path may be influenced by external input. :(

These kind of "just in case" checks all add up.

But I won't insist on a change.

openjdk · 2022-10-26T07:33:52Z

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

8295889: NMT preinit code does not handle allocation errors

Reviewed-by: dholmes, mbaesken

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

772be2e: 8295868: 32-bit Windows build failures after JDK-8294466
e173fd7: 8295885: GHA: Bump gcc versions
b2878c9: 8295710: remove os::dll_file_extension
e3a5a34: 8295767: Remove unused fields in sun.awt.geom.Edge
324bec1: 8295863: JFR: Use expected size for Maps and Sets
3bd3caf: 8294486: Remove vmTestbase/nsk/jvmti/ tests ported to serviceability/jvmti.
fec6174: 8295816: jdwp jck tests failing with "FATAL ERROR in native method: JDWP SetTag, jvmtiError=JVMTI_ERROR_WRONG_PHASE(112)"
e0c2930: 8295282: Use Zicboz/cbo.zero to zero-out memory on RISC-V
d393e05: 8294989: ResourceBundle naming convention issue in JdbcRowSetResourceBundle.java
0caea81: 8295895: build error after JDK-8279366
... and 82 more: https://git.openjdk.org/jdk/compare/1d883c5312721980898f91898665b528948a985b...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.

tstuefe · 2022-10-26T07:50:08Z

I'm more concerned that we don't know if this path may be influenced by external input. :(

The only randomness source I can think of is arguments. Argument handling is done before parsing and therefore before NMT initialization in preinit time. It influences the number of allocated blocks as well as their size. So if someone were to pass an argument close to size_max to the VM, it would run into overflow :-) but that is not possible, of course.

These kind of "just in case" checks all add up.

True. But as this is security relevant, I'll leave it in. I rather shave off time somewhere else. E.g. in the real os::malloc, there are some optimizations we can do. E.g. getting rid of the atomic allocation counters in os.cpp since we do have NMT counting and malloc limits now. That is one thing I have on my list.

MBaesken

I would prefer more detailed guarantee output like for example
guarantee(outer_size > payload_size, "Overflow - %zu must be larger than payload size %zu", outer_size, payload_size);
But it is okay as it is too, approved.

tstuefe · 2022-10-26T08:33:38Z

I would prefer more detailed guarantee output like for example guarantee(outer_size > payload_size, "Overflow - %zu must be larger than payload size %zu", outer_size, payload_size); But it is okay as it is too, approved.

Thanks Matthias! I expect this never to fire, and if it does, someone needs to dive in anyway.

tstuefe · 2022-10-26T08:33:45Z

/integrate

openjdk · 2022-10-26T08:35:15Z

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

772be2e: 8295868: 32-bit Windows build failures after JDK-8294466
e173fd7: 8295885: GHA: Bump gcc versions
b2878c9: 8295710: remove os::dll_file_extension
e3a5a34: 8295767: Remove unused fields in sun.awt.geom.Edge
324bec1: 8295863: JFR: Use expected size for Maps and Sets
3bd3caf: 8294486: Remove vmTestbase/nsk/jvmti/ tests ported to serviceability/jvmti.
fec6174: 8295816: jdwp jck tests failing with "FATAL ERROR in native method: JDWP SetTag, jvmtiError=JVMTI_ERROR_WRONG_PHASE(112)"
e0c2930: 8295282: Use Zicboz/cbo.zero to zero-out memory on RISC-V
d393e05: 8294989: ResourceBundle naming convention issue in JdbcRowSetResourceBundle.java
0caea81: 8295895: build error after JDK-8279366
... and 82 more: https://git.openjdk.org/jdk/compare/1d883c5312721980898f91898665b528948a985b...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-10-26T08:35:31Z

@tstuefe Pushed as commit cf121df.

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

JDK-8295889-NMT-preinit-should-handle-allocation-errors

76d9d5b

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Oct 25, 2022

tstuefe marked this pull request as ready for review October 25, 2022 16:17

openjdk bot added the rfr Pull request is ready for review label Oct 25, 2022

dholmes-ora reviewed Oct 26, 2022

View reviewed changes

assert overflows (debug+release)

ac9c336

dholmes-ora reviewed Oct 26, 2022

View reviewed changes

dholmes-ora approved these changes Oct 26, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 26, 2022

MBaesken approved these changes Oct 26, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 26, 2022

openjdk bot closed this Oct 26, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8295889: NMT preinit code does not handle allocation errors #10855

JDK-8295889: NMT preinit code does not handle allocation errors #10855

tstuefe commented Oct 25, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 25, 2022

openjdk bot commented Oct 25, 2022

tstuefe commented Oct 25, 2022

mlbridge bot commented Oct 25, 2022 •

edited

Loading

dholmes-ora left a comment

tstuefe commented Oct 26, 2022

dholmes-ora commented Oct 26, 2022

dholmes-ora Oct 26, 2022

tstuefe Oct 26, 2022

dholmes-ora Oct 26, 2022

dholmes-ora Oct 26, 2022

openjdk bot commented Oct 26, 2022 •

edited

Loading

tstuefe commented Oct 26, 2022 •

edited

Loading

MBaesken left a comment

tstuefe commented Oct 26, 2022

tstuefe commented Oct 26, 2022

openjdk bot commented Oct 26, 2022

openjdk bot commented Oct 26, 2022

JDK-8295889: NMT preinit code does not handle allocation errors #10855

JDK-8295889: NMT preinit code does not handle allocation errors #10855

Conversation

tstuefe commented Oct 25, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 25, 2022

openjdk bot commented Oct 25, 2022

tstuefe commented Oct 25, 2022

mlbridge bot commented Oct 25, 2022 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

tstuefe commented Oct 26, 2022

dholmes-ora commented Oct 26, 2022

dholmes-ora Oct 26, 2022

Choose a reason for hiding this comment

tstuefe Oct 26, 2022

Choose a reason for hiding this comment

dholmes-ora Oct 26, 2022

Choose a reason for hiding this comment

dholmes-ora Oct 26, 2022

Choose a reason for hiding this comment

openjdk bot commented Oct 26, 2022 • edited Loading

tstuefe commented Oct 26, 2022 • edited Loading

MBaesken left a comment

Choose a reason for hiding this comment

tstuefe commented Oct 26, 2022

tstuefe commented Oct 26, 2022

openjdk bot commented Oct 26, 2022

openjdk bot commented Oct 26, 2022

tstuefe commented Oct 25, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 25, 2022 •

edited

Loading

openjdk bot commented Oct 26, 2022 •

edited

Loading

tstuefe commented Oct 26, 2022 •

edited

Loading