5041655: (ch) FileLock: negative param and overflow issues #7254

bplb · 2022-01-27T22:12:32Z

Add an implementation note to java.nio.channels.FileLock.overlaps(long,long) indicating that the method does not check its parameters. Adding such checks would be an incompatible change.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed
Change requires a CSR request to be approved

Issues

JDK-5041655: (ch) FileLock: negative param and overflow issues
JDK-8281623: (ch) FileLock: negative param and overflow issues (CSR)

Reviewers

Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to ef5b46e

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7254

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

Using diff file

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

bridgekeeper · 2022-01-27T22:13:35Z

👋 Welcome back bpb! 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-01-27T22:16:14Z

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

nio

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 · 2022-01-27T22:19:37Z

Webrevs

AlanBateman · 2022-01-28T05:51:34Z

If the javadoc is changed to document long standing behavior then the change would need to be normative, I don't think an impl note works here. I agree that throwing IAE is not an option. However, maybe you could try this:

Return false if size is negative, such a region can't overlap with with the region of any file lock.
If position is negative then treat it an overlap request on (0, size + position). That would mean an overlap on the positive range. This part may not require any code changes btw, the existing check might over it.

bplb · 2022-01-28T17:21:11Z

I thought through several possibilities, including size < 0 implies false and using Math.addExact() with position and size. If Math.addExact(position, size) throws then the exception would be ignored.
I don't understand what you intend here. Was it instead maybe overlaps(position, size) for position < 0 becomes overlaps(0, size)?

AlanBateman · 2022-01-28T20:13:19Z

I thought through several possibilities, including size < 0 implies false and using Math.addExact() with position and size. If Math.addExact(position, size) throws then the exception would be ignored.

I don't understand what you intend here. Was it instead maybe overlaps(position, size) for position < 0 becomes overlaps(0, size)?

It might be that checking if size <= 0 is enough. Maybe the starting point for this issue is a series of test cases with negative positions and sizes. All tests that uses a negative size should return false. The tests that use a negative position may overlap with the locked region.

bplb · 2022-01-29T04:08:16Z

Never mind about #2: I know what you mean.

bplb · 2022-01-31T17:52:10Z

I already wrote a test but it was not sufficiently cleaned up to include yet.

…es; handle overflow; handle zero size on Windows; add test

bplb · 2022-02-05T00:19:40Z

Updates in commit 01:

FileChannel lock() and tryLock() now indicate that a lock of size zero means lock to end of file.
The FileLock constructors now also state that size zero is to end of file.
FileLock.overlaps() was revised.
Windows native locking functions now recognize size zero as signifying an unbounded range.
A test was added.

bplb · 2022-02-05T00:38:55Z

/csr

openjdk · 2022-02-05T00:39:53Z

@bplb has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@bplb please create a CSR request for issue JDK-5041655. This pull request cannot be integrated until the CSR request is approved.

AlanBateman · 2022-02-06T16:45:33Z

Changing the overlaps method to return false when size is negative is good. The updated range check that allows for the position to be negative looks good too. The compatibility impact should be negligible for this part.

Changing the lock method to treat size==0 as Long.MAX_VALUE-position is the behavior on some platforms already and is implementable in a consistent way on all platforms. So on the surface it's probably the right thing to do. However as proposed, it creates a FileLock with size == 0 that will not overlap with any other lock. To work correctly, it will need to be "promoted" to Long.MAX_VALUE-position before creating the FileLock. That will allow you to drop the change to the native code from the patch.

I have several comments on the javadoc changes but I think we need to get to agreement on size=0 first.

bplb · 2022-02-08T18:04:03Z

I don't understand your comments about size == 0 in the second paragraph. The changes to FileLock::overlaps treat zero size as effectively infinite. The change to the Windows version of FileDispatcherImpl.c brings it functionally in line with the Unix version, although on Windows size has to be promoted whereas on Unix if it is Long.MAX_VALUE it is changed to zero in accord with fcntl().

AlanBateman · 2022-02-08T18:19:02Z

The change to the Windows version of FileDispatcherImpl.c brings it functionally in line with the Unix version, although on Windows size has to be promoted whereas on Unix if it is Long.MAX_VALUE it is changed to zero in accord with fcntl().

With the proposal, fc.lock(pos, 0L, shared) returns a FileLock where size() returns 0L. Can you try having this create a FileLock that reports its size as Long.MAX_VALUE - pos ? That should eliminate the inconsistencies in the proposal and the native code changes will go away too.

bplb · 2022-02-08T18:20:51Z

I can change it as suggested. Note that the extant Unix version for size zero creates a lock where size() returns zero while in reality locking the whole file.

AlanBateman · 2022-02-08T18:27:23Z

Note that the extant Unix version for size zero creates a lock where size() returns zero while in reality locking the whole file.

Right, so you end up with a FileLock that says it doesn't overlap with any other lock but it actually locks the region to the end of file. I think the inconsistencies will go if you promote 0 to MAX_VALUE-pos before create the FileLock object.

bplb · 2022-02-08T18:32:06Z

I think that overlaps() as-is would return true if the size were left at zero, but returning zero from size() does seem strange.

… change

bplb · 2022-02-08T19:21:04Z

Commit 02: suggested source changes made; javadoc not yet changed.

AlanBateman · 2022-02-08T19:59:15Z

Commit 02: suggested source changes made; javadoc not yet changed.

I think the change will be in FileChannelImpl.lock so that the same size is used to create the FileLock and passed through to the native dispatcher.

bplb · 2022-02-08T20:09:10Z

Note that because of this code in the Unix FileDispatcherImpl.c setting size to MAX_VALUE - position is not quite the same as what is there now:

    fl.l_whence = SEEK_SET;
    if (size == (jlong)java_lang_Long_MAX_VALUE) {
        fl.l_len = (off64_t)0;
    } else {
        fl.l_len = (off64_t)size;
    }

AlanBateman · 2022-02-08T20:18:03Z

Note that because of this code in the Unix FileDispatcherImpl.c setting size to MAX_VALUE - position is not quite the same as what is there now:
    fl.l_whence = SEEK_SET;
    if (size == (jlong)java_lang_Long_MAX_VALUE) {
        fl.l_len = (off64_t)0;
    } else {
        fl.l_len = (off64_t)size;
    }

There should be no impact here. If someone calls fc.lock() then it's the equivalent of fc.lock(0, MAX_VALUE, false). The FileLock will be created with size=MAX_VALUE. The code above will just map it to 0 for the call to fcntl.

AlanBateman · 2022-02-09T14:17:56Z

Thanks for the update, I think this change is on the right track now.

For FileChannel.lock we'll need to come up with wording that precisely describes the behavior for the size==0 case. The issue with phrases like "remainder of the file" is that file may be extended after locking.

The update to the FileLock protected constructors will need attention too. Extending FileLock and calling the constructor with size==0 doesn't align the proposed spec change (there is no connection to the native dispatching when creating a FileLock like this). I think it will work if you drop "A value of zero indicates the remainder of the file" from the javadoc.

…leLock spec

src/java.base/share/classes/java/nio/channels/FileLock.java

src/java.base/share/classes/java/nio/channels/FileChannel.java

bplb · 2022-02-11T01:42:57Z

In the Unix version of Java_sun_nio_ch_FileDispatcherImpl_lock0(), I wonder whether in this block

    if (size == (jlong)java_lang_Long_MAX_VALUE) {
        fl.l_len = (off64_t)0;
    } else {
        fl.l_len = (off64_t)size;
    }

the check should be changed to

    if (pos + size == (jlong)java_lang_Long_MAX_VALUE) {

Similar question for release0().

bplb · 2022-02-11T02:00:50Z

CSR JDK-8281623 filed.

src/java.base/share/classes/java/nio/channels/AsynchronousFileChannel.java

…MAX_VALUE - position

…ze zero yields size Long.MAX_VALUE - position

src/java.base/share/classes/sun/nio/ch/SimpleAsynchronousFileChannelImpl.java

src/java.base/share/classes/java/nio/channels/FileLock.java

openjdk · 2022-02-18T15:19:45Z

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

5041655: (ch) FileLock: negative param and overflow issues

Reviewed-by: alanb

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

d3749de: 8277488: Add expiry exception for Digicert (geotrustglobalca) expiring in May 2022
3943c89: 8282044: [JVMCI] Export _sha3_implCompress, _md5_implCompress and aarch64::_has_negatives stubs to JVMCI compiler.
7ce75af: 8255266: Update Public Suffix List to 3c213aa
cfbfd9b: 8282103: fix macosx-generic typo in ProblemList
413bef6: 8282049: AArch64: Use ZR for integer zero immediate volatile stores
cf6984d: 8282086: Update jib profile to not set build to 0
f5120b7: 8282056: Clean up com.sun.tools.javac.util.GraphUtils
e336504: 8280866: SuppressWarnings does not work properly in package-info and module-info
e8224f7: 8282089: [BACKOUT] Parallel: Refactor PSCardTable::scavenge_contents_parallel
834d55c: 8277300: Issues with javadoc support for preview features
... and 293 more: https://git.openjdk.java.net/jdk/compare/8e82d0021c119b7793870811fad37d7659c1174d...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.

bplb · 2022-02-22T17:23:04Z

/integrate

openjdk · 2022-02-22T17:24:26Z

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

7feabee: 8261407: ReflectionFactory.checkInitted() is not thread-safe
58e1882: 8282042: [testbug] FileEncodingTest.java depends on default encoding
3cb3867: 8281315: Unicode, (?i) flag and backreference throwing IndexOutOfBounds Exception
957dae0: 8280958: G1/Parallel: Unify marking code structure
e44d067: 8244593: Clean up GNM/NM after JEP 381
41355e2: 8276686: Malformed Javadoc inline tags in JDK source in /java/util/regex/Pattern.java
022d807: 8271008: appcds/*/MethodHandlesAsCollectorTest.java tests time out because of excessive GC (CodeCache GC Threshold) in loom
ab6d8e6: 8260328: Drop redundant CSS properties from java.desktop HTML files
b95310b: 8282220: contentType should not be a PKCS7's member
bc43320: 8281543: Remove unused code/headerfile dtraceAttacher.hpp
... and 317 more: https://git.openjdk.java.net/jdk/compare/8e82d0021c119b7793870811fad37d7659c1174d...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-22T17:24:41Z

@bplb Pushed as commit 6445ee4.

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

5041655: (ch) FileLock: negative param and overflow issues

3916723

openjdk bot added the rfr Pull request is ready for review label Jan 27, 2022

openjdk bot added the nio [email protected] label Jan 27, 2022

5041655: Remove implNote; add special cases for negative and zero siz…

f97a412

…es; handle overflow; handle zero size on Windows; add test

openjdk bot added the csr Pull request needs approved CSR before integration label Feb 5, 2022

5041655: Force zero size of Long.MAX_VALUE - position; revert Windows…

30ce90c

… change

5041655: Move size zero handling to file channel impl classes

d1001cc

bplb force-pushed the FileLock-overlaps-5041655 branch from e5d88e2 to d1001cc Compare February 9, 2022 02:54

5041655: Update spec of zero lock size in channels and remove from Fi…

1e12457

…leLock spec

AlanBateman reviewed Feb 10, 2022

View reviewed changes

src/java.base/share/classes/java/nio/channels/FileLock.java Outdated Show resolved Hide resolved

AlanBateman reviewed Feb 10, 2022

View reviewed changes

src/java.base/share/classes/java/nio/channels/FileChannel.java Show resolved Hide resolved

5041655: Further refinement of zero size lock specification

8e0de59

AlanBateman reviewed Feb 11, 2022

View reviewed changes

src/java.base/share/classes/java/nio/channels/AsynchronousFileChannel.java Outdated Show resolved Hide resolved

bplb added 3 commits February 11, 2022 07:55

5041655: Update description of when a lock is created with size Long.…

d56daa9

…MAX_VALUE - position

5041655: Modify existing tests to verify that creating a lock with si…

537525e

…ze zero yields size Long.MAX_VALUE - position

5041655: Fix problem exposed by modified lock tests

f1e1dfa

openjdk bot removed the csr Pull request needs approved CSR before integration label Feb 16, 2022

AlanBateman reviewed Feb 17, 2022

View reviewed changes

src/java.base/share/classes/sun/nio/ch/SimpleAsynchronousFileChannelImpl.java Outdated Show resolved Hide resolved

AlanBateman reviewed Feb 17, 2022

View reviewed changes

src/java.base/share/classes/java/nio/channels/FileLock.java Show resolved Hide resolved

5041655: Move x is below y comments

ef5b46e

AlanBateman approved these changes Feb 18, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 18, 2022

5041655: Add parentheses around boolean in ternary expression

cbcdcf0

openjdk bot added the integrated Pull request has been integrated label Feb 22, 2022

openjdk bot closed this Feb 22, 2022

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

bplb deleted the FileLock-overlaps-5041655 branch March 14, 2022 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

5041655: (ch) FileLock: negative param and overflow issues #7254

5041655: (ch) FileLock: negative param and overflow issues #7254

bplb commented Jan 27, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 27, 2022

openjdk bot commented Jan 27, 2022

mlbridge bot commented Jan 27, 2022 •

edited

Loading

AlanBateman commented Jan 28, 2022

bplb commented Jan 28, 2022

AlanBateman commented Jan 28, 2022

bplb commented Jan 29, 2022

bplb commented Jan 31, 2022

bplb commented Feb 5, 2022

bplb commented Feb 5, 2022

openjdk bot commented Feb 5, 2022

AlanBateman commented Feb 6, 2022 •

edited

Loading

bplb commented Feb 8, 2022 •

edited

Loading

AlanBateman commented Feb 8, 2022

bplb commented Feb 8, 2022 •

edited

Loading

AlanBateman commented Feb 8, 2022

bplb commented Feb 8, 2022

bplb commented Feb 8, 2022

AlanBateman commented Feb 8, 2022

bplb commented Feb 8, 2022

AlanBateman commented Feb 8, 2022

AlanBateman commented Feb 9, 2022 •

edited

Loading

bplb commented Feb 11, 2022

bplb commented Feb 11, 2022

openjdk bot commented Feb 18, 2022 •

edited

Loading

bplb commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

5041655: (ch) FileLock: negative param and overflow issues #7254

5041655: (ch) FileLock: negative param and overflow issues #7254

Conversation

bplb commented Jan 27, 2022 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Jan 27, 2022

openjdk bot commented Jan 27, 2022

mlbridge bot commented Jan 27, 2022 • edited Loading

Webrevs

AlanBateman commented Jan 28, 2022

bplb commented Jan 28, 2022

AlanBateman commented Jan 28, 2022

bplb commented Jan 29, 2022

bplb commented Jan 31, 2022

bplb commented Feb 5, 2022

bplb commented Feb 5, 2022

openjdk bot commented Feb 5, 2022

AlanBateman commented Feb 6, 2022 • edited Loading

bplb commented Feb 8, 2022 • edited Loading

AlanBateman commented Feb 8, 2022

bplb commented Feb 8, 2022 • edited Loading

AlanBateman commented Feb 8, 2022

bplb commented Feb 8, 2022

bplb commented Feb 8, 2022

AlanBateman commented Feb 8, 2022

bplb commented Feb 8, 2022

AlanBateman commented Feb 8, 2022

AlanBateman commented Feb 9, 2022 • edited Loading

bplb commented Feb 11, 2022

bplb commented Feb 11, 2022

openjdk bot commented Feb 18, 2022 • edited Loading

bplb commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

bplb commented Jan 27, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 27, 2022 •

edited

Loading

AlanBateman commented Feb 6, 2022 •

edited

Loading

bplb commented Feb 8, 2022 •

edited

Loading

bplb commented Feb 8, 2022 •

edited

Loading

AlanBateman commented Feb 9, 2022 •

edited

Loading

openjdk bot commented Feb 18, 2022 •

edited

Loading