8256865: Foreign Memory Access and Linker API are missing NPE checks #1388

mcimadamore · 2020-11-23T15:11:49Z

Both the Foreign Memory Access and the Foreign Linker APIs leave something to be desired when it comes to handling NPEs - first, most of the API javadoc is oblivious to NPEs being thrown. Secondly, not all API method implementations add expicit NPE checks - with the result of NPE often being thrown very deep in the call chain - if at all. Third, test for API coverage of nulls is ad-hoc.

This patch rectifies all these issues. To increase coverage for null injected into APIs, this patch introduces a new framework for testing an API in bulk, so that all methods are reflectively called with some values replaced with nulls, so that all combinations are tried.

I've also added, as part of this patch, a test to cover the statics in MemoryAccess which were not covered throughly.

Progress

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

Testing

	Linux x64	Linux x86	Windows x64	macOS x64
Build	❌ (1/6 failed)	❌ (2/2 failed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)			✔️ (9/9 passed)	✔️ (9/9 passed)

Failed test tasks

Issue

JDK-8256865: Foreign Memory Access and Linker API are missing NPE checks

Reviewers

Jorn Vernee (@JornVernee - Committer) ⚠️ Review applies to 80ffde4
Athijegannathan Sundararajan (@sundararajana - Reviewer) ⚠️ Review applies to 4565f77
Chris Hegarty (@ChrisHegarty - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1388/head:pull/1388
$ git checkout pull/1388

bridgekeeper · 2020-11-23T15:13:03Z

👋 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 · 2020-11-23T15:14:13Z

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

core-libs

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 · 2020-11-23T15:19:08Z

Webrevs

mcimadamore · 2020-11-23T15:19:39Z

/csr

openjdk · 2020-11-23T15:19:57Z

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

mcimadamore · 2020-11-23T15:21:06Z

CSR: https://bugs.openjdk.java.net/browse/JDK-8256866
specdiff: http://cr.openjdk.java.net/~mcimadamore/8256865_v1/specdiff_out/overview-summary.html
javadoc: http://cr.openjdk.java.net/~mcimadamore/8256865_v1/javadoc

JornVernee

Already looked at this in panama-foreign, but found one minor issue.

JornVernee · 2020-11-23T17:58:22Z

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/FunctionDescriptor.java

@@ -140,9 +148,11 @@ public static FunctionDescriptor ofVoid(MemoryLayout... argLayouts) {
     * of this function descriptor.
     * @param addedLayouts the argument layouts to append.
     * @return the new function descriptor.
-     * @throws NullPointerException if any of the new argument layouts is null.
+     * @throws NullPointerException if either {@code addedLayouts == null}, or any of the
+     * layouts in {@code addedLayouts} is null.@throws NullPointerException if any of the new argument layouts is null.


Spurious @throws tag here

dholmes-ora · 2020-11-24T02:24:57Z

Just an observation but there are precedents for just placing a generic statement in the package and/or class javadoc:

"Unless specified otherwise any method that is passed a null reference will throw NullPointerException"

rather than adding @throws NPE all over the place.

sundararajana

Unused imports in TestLayouts.java?

Add test support for LibraryLookup.

mcimadamore · 2020-11-24T15:38:17Z

Following @dholmes-ora suggestion and also @jddarcy (in CSR review), I've uploaded a new iteration which instead of adding a @throws tag at every method, adds a general statement on each class which gets the treatment. I think specifying at package-level seems a bit too loose, so I went for per-class basis.

mcimadamore · 2020-11-24T16:00:52Z

specdiff: http://cr.openjdk.java.net/~mcimadamore/8256865_v2/specdiff_out/overview-summary.html
javadoc: http://cr.openjdk.java.net/~mcimadamore/8256865_v2/javadoc

openjdk · 2020-11-25T00:29:34Z

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

8256865: Foreign Memory Access and Linker API are missing NPE checks

Reviewed-by: jvernee, sundar, chegar

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

8cd2e0f: 8243315: ParallelScavengeHeap::initialize() passes GenAlignment as page size to os::trace_page_sizes instead of actual page size
cdb41ba: 8255904: Remove superfluous use of reflection in Class::isRecord
c45725e: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing
79e57ac: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, setDaemon and isDaemon
0d91f0a: 8252848: Optimize small primitive arrayCopy operations through partial inlining using AVX-512 masked instructions
66943fe: 8256517: (ref) Reference.clear during reference processing may lose notification
3c230b8: 8256993: Clarify Package::isSealed javadoc about package sealing vs sealed class or interface
1b7a61f: 8254999: Move G1RemSetSamplingTask to more appropriate location
695117f: 8255479: [aarch64] assert(src->section_index_of(target) == CodeBuffer::SECT_NONE) failed: sanity
dbfeb90: 8243559: Remove root certificates with 1024-bit keys
... and 33 more: https://git.openjdk.java.net/jdk/compare/84429cd944b771f5da5d1f1903bf2248f22fd8c7...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.

mcimadamore · 2020-11-25T10:47:48Z

/integrate

openjdk · 2020-11-25T10:49:07Z

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

8cd2e0f: 8243315: ParallelScavengeHeap::initialize() passes GenAlignment as page size to os::trace_page_sizes instead of actual page size
cdb41ba: 8255904: Remove superfluous use of reflection in Class::isRecord
c45725e: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing
79e57ac: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, setDaemon and isDaemon
0d91f0a: 8252848: Optimize small primitive arrayCopy operations through partial inlining using AVX-512 masked instructions
66943fe: 8256517: (ref) Reference.clear during reference processing may lose notification
3c230b8: 8256993: Clarify Package::isSealed javadoc about package sealing vs sealed class or interface
1b7a61f: 8254999: Move G1RemSetSamplingTask to more appropriate location
695117f: 8255479: [aarch64] assert(src->section_index_of(target) == CodeBuffer::SECT_NONE) failed: sanity
dbfeb90: 8243559: Remove root certificates with 1024-bit keys
... and 33 more: https://git.openjdk.java.net/jdk/compare/84429cd944b771f5da5d1f1903bf2248f22fd8c7...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9aeadbb.

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

Reviewed-by: jvernee, sundar, chegar

mcimadamore added 7 commits November 16, 2020 21:34

Create specialized subclasses for all the heap segment cases

412b209

make base() method abstract in HeapMemorySegmentImpl

93fe301

Merge branch 'master' into npe-fixes

a730599

Fix memory access NPEs

488fe4c

Fix memory access NPEs cont'd

148a78f

Merge branch 'master' into npe-fixes

46501f5

Add NPE checks in foreign ABI

80ffde4

openjdk bot added the rfr Pull request is ready for review label Nov 23, 2020

openjdk bot added the core-libs core-libs-dev@openjdk.org label Nov 23, 2020

openjdk bot added the csr Pull request needs approved CSR before integration label Nov 23, 2020

JornVernee approved these changes Nov 23, 2020

View reviewed changes

Fix cut/paste error in FunctionDescriptor

4565f77

sundararajana approved these changes Nov 24, 2020

View reviewed changes

ChrisHegarty approved these changes Nov 24, 2020

View reviewed changes

mcimadamore added 2 commits November 24, 2020 15:06

Fix unused import

54d5e14

Add coarser-grained statements abut NPE handling at the class level.

bbdff9f

Add test support for LibraryLookup.

mcimadamore added 3 commits November 24, 2020 15:44

Fix spurious typos caused by bulk removal of @throws tag

19a77d9

Fix use of star import in LibrariesHelper

6c25c73

Fix another typo caused by bulk replace

40c2d20

openjdk bot removed the csr Pull request needs approved CSR before integration label Nov 25, 2020

openjdk bot added the ready Pull request is ready to be integrated label Nov 25, 2020

ChrisHegarty approved these changes Nov 25, 2020

View reviewed changes

openjdk bot closed this Nov 25, 2020

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 Nov 25, 2020

openjdk-notifier bot referenced this pull request Nov 25, 2020

8256865: Foreign Memory Access and Linker API are missing NPE checks

9aeadbb

Reviewed-by: jvernee, sundar, chegar

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8256865: Foreign Memory Access and Linker API are missing NPE checks #1388

8256865: Foreign Memory Access and Linker API are missing NPE checks #1388

mcimadamore commented Nov 23, 2020 •

edited by openjdk bot

bridgekeeper bot commented Nov 23, 2020

openjdk bot commented Nov 23, 2020

mlbridge bot commented Nov 23, 2020 •

edited

mcimadamore commented Nov 23, 2020

openjdk bot commented Nov 23, 2020

mcimadamore commented Nov 23, 2020

JornVernee left a comment

JornVernee Nov 23, 2020

mcimadamore Nov 23, 2020

dholmes-ora commented Nov 24, 2020

sundararajana left a comment

mcimadamore commented Nov 24, 2020

mcimadamore commented Nov 24, 2020

openjdk bot commented Nov 25, 2020 •

edited

mcimadamore commented Nov 25, 2020

openjdk bot commented Nov 25, 2020

8256865: Foreign Memory Access and Linker API are missing NPE checks #1388

8256865: Foreign Memory Access and Linker API are missing NPE checks #1388

Conversation

mcimadamore commented Nov 23, 2020 • edited by openjdk bot

Progress

Testing

Issue

Reviewers

Download

bridgekeeper bot commented Nov 23, 2020

openjdk bot commented Nov 23, 2020

mlbridge bot commented Nov 23, 2020 • edited

Webrevs

mcimadamore commented Nov 23, 2020

openjdk bot commented Nov 23, 2020

mcimadamore commented Nov 23, 2020

JornVernee left a comment

Choose a reason for hiding this comment

JornVernee Nov 23, 2020

Choose a reason for hiding this comment

mcimadamore Nov 23, 2020

Choose a reason for hiding this comment

dholmes-ora commented Nov 24, 2020

sundararajana left a comment

Choose a reason for hiding this comment

mcimadamore commented Nov 24, 2020

mcimadamore commented Nov 24, 2020

openjdk bot commented Nov 25, 2020 • edited

mcimadamore commented Nov 25, 2020

openjdk bot commented Nov 25, 2020

mcimadamore commented Nov 23, 2020 •

edited by openjdk bot

mlbridge bot commented Nov 23, 2020 •

edited

openjdk bot commented Nov 25, 2020 •

edited