# HG changeset patch # User bpb # Date 1478546302 28800 # Mon Nov 07 11:18:22 2016 -0800 # Node ID 3c850415d287abbed3d1c28c592f26a8dfbbf117 # Parent 762dad36483839e4b53ab698d93f047e2990b8da 8166253: (ch) FileLock object can get GC'd and result in unexpected release of file lock Summary: XXX Reviewed-by: XXX diff --git a/test/java/nio/channels/FileLock/FileLockGC.java b/test/java/nio/channels/FileLock/FileLockGC.java new file mode 100644 --- /dev/null +++ b/test/java/nio/channels/FileLock/FileLockGC.java @@ -0,0 +1,154 @@ +/* + * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +import java.io.File; +import java.io.IOException; +import java.io.RandomAccessFile; +import java.lang.ref.Reference; +import java.lang.ref.WeakReference; +import java.nio.channels.FileLock; +import java.nio.channels.OverlappingFileLockException; +import java.nio.file.Files; +import java.nio.file.Path; + +/* + * @test + * @bug 8166253 + * @summary Verify that OverlappingFileLockException is thrown when expected. + */ +public class FileLockGC { + public enum TestType { + DEFAULT(true), + GC(true), + RELEASE(false), + GC_THEN_RELEASE(false), + RELEASE_THEN_GC(false); + + private final boolean exceptionExpected; + + TestType(boolean exceptionExpected) { + this.exceptionExpected = exceptionExpected; + } + + boolean exceptionExpected() { + return exceptionExpected; + } + } + + public static void main(String[] args) throws Exception { + final File f = new File(System.getProperty("test.dir", ".") + + File.separator + "junk.txt"); + final Path p = f.toPath(); + int failures = 0; + + for (TestType t : TestType.values()) { + try { + if (!testFileLockGC(f, t)) { + failures++; + } + } finally { + Files.delete(p); + } + } + + if (failures != 0) { + throw new RuntimeException("Test had " + failures + " failure(s)"); + } + } + + private static void gc() { + System.out.println("Performing GC"); + System.gc(); + } + + private static void release(Reference ref) throws IOException { + Object lock = ref.get(); + if (lock != null) { + System.out.println("Releasing lock"); + ((FileLock)lock).release(); + } + } + + private static boolean testFileLockGC(File f, TestType type) + throws IOException { + System.out.printf("Test %s starting%n", type.toString()); + + final RandomAccessFile raf1 = new RandomAccessFile(f, "rw"); + + WeakReference ref1 = new WeakReference(raf1.getChannel().tryLock()); + + switch (type) { + case GC: + gc(); + break; + case RELEASE: + release(ref1); + break; + case GC_THEN_RELEASE: + // this case should usually be the same as the GC as the + // reference should have been cleared + gc(); + release(ref1); + break; + case RELEASE_THEN_GC: + release(ref1); + gc(); + break; + default: + break; + } + + final RandomAccessFile raf2 = new RandomAccessFile(f, "rw"); + + boolean success = true; + WeakReference ref2 = null; + try { + ref2 = new WeakReference(raf2.getChannel().tryLock()); + if (type.exceptionExpected()) { + System.err.printf + ("No expected OverlappingFileLockException for test %s%n", + type.toString()); + success = false; + } + } catch (OverlappingFileLockException ofe) { + if (!type.exceptionExpected()) { + System.err.printf + ("Unexpected OverlappingFileLockException for test %s%n", + type.toString()); + success = false; + } + } finally { + if (type == TestType.DEFAULT | type == TestType.GC) { + release(ref1); + } + if (ref2 != null) { + release(ref2); + } + raf2.close(); + raf1.close(); + System.out.printf("Test %s finished%n", type.toString()); + } + + return success; + } +} # HG changeset patch # User bpb # Date 1478546302 28800 # Mon Nov 07 11:18:22 2016 -0800 # Node ID 7eb05ede4694e9c09972d72e94302dddd80b493b # Parent 3c850415d287abbed3d1c28c592f26a8dfbbf117 imported patch FileLockTable.01 diff --git a/src/java.base/share/classes/sun/nio/ch/FileLockImpl.java b/src/java.base/share/classes/sun/nio/ch/FileLockImpl.java --- a/src/java.base/share/classes/sun/nio/ch/FileLockImpl.java +++ b/src/java.base/share/classes/sun/nio/ch/FileLockImpl.java @@ -32,6 +32,7 @@ extends FileLock { private volatile boolean invalid; + private volatile FileLockState state; FileLockImpl(FileChannel channel, long position, long size, boolean shared) { @@ -43,6 +44,14 @@ super(channel, position, size, shared); } + FileLockState getState() { + FileLockState s = state; + if (s == null) { + state = s = new FileLockState(position(), size(), invalid); + } + return s; + } + public boolean isValid() { return !invalid; } @@ -50,6 +59,9 @@ void invalidate() { assert Thread.holdsLock(this); invalid = true; + if (state != null) { + state.invalidate(); + } } public synchronized void release() throws IOException { diff --git a/src/java.base/share/classes/sun/nio/ch/FileLockState.java b/src/java.base/share/classes/sun/nio/ch/FileLockState.java new file mode 100644 --- /dev/null +++ b/src/java.base/share/classes/sun/nio/ch/FileLockState.java @@ -0,0 +1,55 @@ +/* + * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Oracle designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +package sun.nio.ch; + +class FileLockState { + final long position; + final long size; + private volatile boolean invalid; + + FileLockState(long position, long size, boolean invalid) { + this.position = position; + this.size = size; + this.invalid = invalid; + } + + public void invalidate() { + invalid = true; + } + + boolean isValid() { + return !invalid; + } + + // Copied from java.nio.channels.FileLock. + final boolean overlaps(long position, long size) { + if (position + size <= this.position) + return false; // That is below this + if (this.position + this.size <= position) + return false; // This is below that + return true; + } +} diff --git a/src/java.base/share/classes/sun/nio/ch/FileLockTable.java b/src/java.base/share/classes/sun/nio/ch/FileLockTable.java --- a/src/java.base/share/classes/sun/nio/ch/FileLockTable.java +++ b/src/java.base/share/classes/sun/nio/ch/FileLockTable.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2005, 2009, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2005, 2016, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -87,17 +87,25 @@ */ private static class FileLockReference extends WeakReference { private FileKey fileKey; + private FileLockState lockState; FileLockReference(FileLock referent, ReferenceQueue queue, FileKey key) { super(referent, queue); this.fileKey = key; + if (referent instanceof FileLockImpl) { + this.lockState = ((FileLockImpl)referent).getState(); + } } FileKey fileKey() { return fileKey; } + + FileLockState lockState() { + return lockState; + } } // The system-wide map is a ConcurrentHashMap that is keyed on the FileKey. @@ -250,8 +258,8 @@ { assert Thread.holdsLock(list); for (FileLockReference ref: list) { - FileLock fl = ref.get(); - if (fl != null && fl.overlaps(position, size)) + FileLockState lockState= ref.lockState(); + if (lockState.isValid() && lockState.overlaps(position, size)) throw new OverlappingFileLockException(); } } diff --git a/test/java/nio/channels/FileLock/FileLockGC.java b/test/java/nio/channels/FileLock/FileLockGC.java --- a/test/java/nio/channels/FileLock/FileLockGC.java +++ b/test/java/nio/channels/FileLock/FileLockGC.java @@ -41,7 +41,7 @@ DEFAULT(true), GC(true), RELEASE(false), - GC_THEN_RELEASE(false), + GC_THEN_RELEASE(true), RELEASE_THEN_GC(false); private final boolean exceptionExpected; @@ -104,9 +104,10 @@ case RELEASE: release(ref1); break; - case GC_THEN_RELEASE: - // this case should usually be the same as the GC as the - // reference should have been cleared + case GC_THEN_RELEASE: + // this case should usually be the same as the GC-only case + // as the reference should have been cleared such that + // release() will have no effect gc(); release(ref1); break; # HG changeset patch # User bpb # Date 1478546302 28800 # Mon Nov 07 11:18:22 2016 -0800 # Node ID 72f68bbacff16796617b417c0afa66a625ade62a # Parent 7eb05ede4694e9c09972d72e94302dddd80b493b imported patch FileLockTable.02 diff --git a/src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java b/src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java --- a/src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java +++ b/src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2000, 2013, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2000, 2016, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -132,6 +132,18 @@ } } + // Release and invalidate any orphaned locks still held + if (fileLockStates != null) { + for (FileLockState fls : fileLockStates) { + synchronized (fls) { + if (fls.isValid()) { + nd.release(fd, fls.position, fls.size); + fls.invalidate(); + } + } + } + } + // signal any threads blocked on this channel threads.signalAndWait(); @@ -1020,6 +1032,9 @@ // has been checked private static volatile boolean propertyChecked; + // States of locks acquired on the channel + private volatile List fileLockStates; + // The lock list in J2SE 1.4/5.0 was local to each FileChannel instance so // the overlap check wasn't system wide when there were multiple channels to // the same file. This property is used to get 1.4/5.0 behavior if desired. @@ -1058,6 +1073,17 @@ return fileLockTable; } + private List fileLockStates() { + if (fileLockStates == null) { + synchronized (this) { + if (fileLockStates == null) { + fileLockStates = new ArrayList(); + } + } + } + return fileLockStates; + } + public FileLock lock(long position, long size, boolean shared) throws IOException { @@ -1069,6 +1095,8 @@ FileLockImpl fli = new FileLockImpl(this, position, size, shared); FileLockTable flt = fileLockTable(); flt.add(fli); + List fls = fileLockStates(); + fls.add(fli.getState()); boolean completed = false; int ti = -1; try { @@ -1086,13 +1114,17 @@ FileLockImpl fli2 = new FileLockImpl(this, position, size, false); flt.replace(fli, fli2); + fls.remove(fli.getState()); fli = fli2; + fls.add(fli.getState()); } completed = true; } } finally { - if (!completed) + if (!completed) { flt.remove(fli); + fls.remove(fli.getState()); + } threads.remove(ti); try { end(completed); @@ -1114,6 +1146,8 @@ FileLockImpl fli = new FileLockImpl(this, position, size, shared); FileLockTable flt = fileLockTable(); flt.add(fli); + List fls = fileLockStates(); + fls.add(fli.getState()); int result; int ti = threads.add(); @@ -1123,10 +1157,12 @@ result = nd.lock(fd, false, position, size, shared); } catch (IOException e) { flt.remove(fli); + fls.remove(fli.getState()); throw e; } if (result == FileDispatcher.NO_LOCK) { flt.remove(fli); + fls.remove(fli.getState()); return null; } if (result == FileDispatcher.RET_EX_LOCK) { @@ -1134,6 +1170,8 @@ FileLockImpl fli2 = new FileLockImpl(this, position, size, false); flt.replace(fli, fli2); + fls.remove(fli.getState()); + fls.add(fli2.getState()); return fli2; } return fli; diff --git a/src/java.base/share/classes/sun/nio/ch/FileLockTable.java b/src/java.base/share/classes/sun/nio/ch/FileLockTable.java --- a/src/java.base/share/classes/sun/nio/ch/FileLockTable.java +++ b/src/java.base/share/classes/sun/nio/ch/FileLockTable.java @@ -117,6 +117,12 @@ // reference queue for cleared refs private static ReferenceQueue queue = new ReferenceQueue(); + // System-wide map of orphaned lock states. A lock state is orphaned when + // the corresponding FileLock is no longer reachable but the underlying + // native lock has not been released. + private static ConcurrentHashMap> orphans = + new ConcurrentHashMap>(); + // The connection to which this table is connected private final Channel channel; @@ -262,6 +268,22 @@ if (lockState.isValid() && lockState.overlaps(position, size)) throw new OverlappingFileLockException(); } + + // XXX Need synchronization here ... + // Check any orphaned states for this key. + List states = orphans.get(fileKey); + if (states != null) { + Iterator iter = states.iterator(); + while (iter.hasNext()) { + FileLockState lockState = iter.next(); + if (lockState.isValid()) { + if (lockState.overlaps(position, size)) + throw new OverlappingFileLockException(); + } else { + iter.remove(); + } + } + } } // Process the reference queue @@ -274,6 +296,23 @@ synchronized (list) { list.remove(ref); removeKeyIfEmpty(fk, list); + FileLockState lockState = ref.lockState(); + if (lockState.isValid()) { + // The native lock is still valid although the + // FileLock instance is no longer reachable. Add + // the state to the list of orphans for this key. + // The FileChannel for the referent is unknown. + // XXX need synchronization here as in add() above. + List states = orphans.get(fk); + boolean isAbsent = states == null; + if (isAbsent) { + states = new ArrayList(2); + } + states.add(lockState); + if (isAbsent) { + orphans.put(fk, states); + } + } } } } # HG changeset patch # User bpb # Date 1478546302 28800 # Mon Nov 07 11:18:22 2016 -0800 # Node ID 2ca06753871f052c394fceb67dbc87820f4e057a # Parent 72f68bbacff16796617b417c0afa66a625ade62a imported patch FileLockTable.03 diff --git a/src/java.base/share/classes/sun/nio/ch/FileLockTable.java b/src/java.base/share/classes/sun/nio/ch/FileLockTable.java --- a/src/java.base/share/classes/sun/nio/ch/FileLockTable.java +++ b/src/java.base/share/classes/sun/nio/ch/FileLockTable.java @@ -184,6 +184,13 @@ } } + private void removeKeyIfStatesEmpty(FileKey fk, List list) { + assert orphans.get(fk) == list; + if (list.isEmpty()) { + orphans.remove(fk); + } + } + @Override public void remove(FileLock fl) { assert fl != null; @@ -265,23 +272,27 @@ assert Thread.holdsLock(list); for (FileLockReference ref: list) { FileLockState lockState= ref.lockState(); - if (lockState.isValid() && lockState.overlaps(position, size)) + if (lockState != null && lockState.isValid() + && lockState.overlaps(position, size)) throw new OverlappingFileLockException(); } - // XXX Need synchronization here ... // Check any orphaned states for this key. List states = orphans.get(fileKey); if (states != null) { - Iterator iter = states.iterator(); - while (iter.hasNext()) { - FileLockState lockState = iter.next(); - if (lockState.isValid()) { - if (lockState.overlaps(position, size)) - throw new OverlappingFileLockException(); - } else { - iter.remove(); + synchronized (states) { + Iterator iter = states.iterator(); + while (iter.hasNext()) { + FileLockState lockState = iter.next(); + if (lockState.isValid()) { + if (lockState.overlaps(position, size)) + throw new OverlappingFileLockException(); + } else { + // Might as well remove invalid state. + iter.remove(); + } } + removeKeyIfStatesEmpty(fileKey, states); } } } @@ -297,20 +308,44 @@ list.remove(ref); removeKeyIfEmpty(fk, list); FileLockState lockState = ref.lockState(); + if (lockState == null) + continue; if (lockState.isValid()) { // The native lock is still valid although the // FileLock instance is no longer reachable. Add // the state to the list of orphans for this key. - // The FileChannel for the referent is unknown. - // XXX need synchronization here as in add() above. + // The FileChannel of the referent is unknown. List states = orphans.get(fk); - boolean isAbsent = states == null; - if (isAbsent) { - states = new ArrayList(2); + while (true) { + if (states == null) { + states = new ArrayList(2); + List prev; + synchronized (states) { + prev = orphans.putIfAbsent(fk, states); + if (prev == null) { + states.add(lockState); + break; + } + } + states = prev; + } + + synchronized (states) { + List current = orphans.get(fk); + if (states == current) { + states.add(lockState); + break; + } + states = current; + } } - states.add(lockState); - if (isAbsent) { - orphans.put(fk, states); + } else { // !lockState.isValid() + List states = orphans.get(fk); + if (states != null) { + synchronized (states) { + states.remove(lockState); + } + removeKeyIfStatesEmpty(fk, states); } } } # HG changeset patch # User bpb # Date 1478553898 28800 # Mon Nov 07 13:24:58 2016 -0800 # Node ID 06690e2dba249f10565c3eab0161d31bd281a744 # Parent 2ca06753871f052c394fceb67dbc87820f4e057a [mq]: FileLockTable.04 diff --git a/src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java b/src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java --- a/src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java +++ b/src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java @@ -42,6 +42,7 @@ import java.nio.channels.WritableByteChannel; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import jdk.internal.misc.JavaIOFileDescriptorAccess; import jdk.internal.misc.JavaNioAccess; @@ -1077,7 +1078,7 @@ if (fileLockStates == null) { synchronized (this) { if (fileLockStates == null) { - fileLockStates = new ArrayList(); + fileLockStates = new CopyOnWriteArrayList(); } } } diff --git a/src/java.base/share/classes/sun/nio/ch/FileLockImpl.java b/src/java.base/share/classes/sun/nio/ch/FileLockImpl.java --- a/src/java.base/share/classes/sun/nio/ch/FileLockImpl.java +++ b/src/java.base/share/classes/sun/nio/ch/FileLockImpl.java @@ -31,37 +31,31 @@ public class FileLockImpl extends FileLock { - private volatile boolean invalid; - private volatile FileLockState state; + private final FileLockState state; FileLockImpl(FileChannel channel, long position, long size, boolean shared) { super(channel, position, size, shared); + state = new FileLockState(position, size); } FileLockImpl(AsynchronousFileChannel channel, long position, long size, boolean shared) { super(channel, position, size, shared); + state = new FileLockState(position, size); } FileLockState getState() { - FileLockState s = state; - if (s == null) { - state = s = new FileLockState(position(), size(), invalid); - } - return s; + return state; } public boolean isValid() { - return !invalid; + return state.isValid(); } void invalidate() { assert Thread.holdsLock(this); - invalid = true; - if (state != null) { - state.invalidate(); - } + state.invalidate(); } public synchronized void release() throws IOException { @@ -74,7 +68,7 @@ else if (ch instanceof AsynchronousFileChannelImpl) ((AsynchronousFileChannelImpl)ch).release(this); else throw new AssertionError(); - invalidate(); + state.invalidate(); } } } diff --git a/src/java.base/share/classes/sun/nio/ch/FileLockState.java b/src/java.base/share/classes/sun/nio/ch/FileLockState.java --- a/src/java.base/share/classes/sun/nio/ch/FileLockState.java +++ b/src/java.base/share/classes/sun/nio/ch/FileLockState.java @@ -30,10 +30,9 @@ final long size; private volatile boolean invalid; - FileLockState(long position, long size, boolean invalid) { + FileLockState(long position, long size) { this.position = position; this.size = size; - this.invalid = invalid; } public void invalidate() { diff --git a/src/java.base/share/classes/sun/nio/ch/FileLockTable.java b/src/java.base/share/classes/sun/nio/ch/FileLockTable.java --- a/src/java.base/share/classes/sun/nio/ch/FileLockTable.java +++ b/src/java.base/share/classes/sun/nio/ch/FileLockTable.java @@ -344,8 +344,8 @@ if (states != null) { synchronized (states) { states.remove(lockState); + removeKeyIfStatesEmpty(fk, states); } - removeKeyIfStatesEmpty(fk, states); } } }