From 2689839d79ce6f862bbe1570ee9bf598381945b4 Mon Sep 17 00:00:00 2001 From: Alan Bateman Date: Fri, 5 Sep 2014 12:18:31 +0100 Subject: [PATCH] 8029516: (fs) WatchKey cancel unreliable on Windows Reviewed-by: chegar --- .../sun/nio/fs/WindowsNativeDispatcher.java | 30 +++++ .../sun/nio/fs/WindowsWatchService.java | 111 +++++++++++----- .../libnio/fs/WindowsNativeDispatcher.c | 48 +++++-- .../nio/file/WatchService/LotsOfCancels.java | 119 ++++++++++++++++++ 4 files changed, 265 insertions(+), 43 deletions(-) create mode 100644 jdk/test/java/nio/file/WatchService/LotsOfCancels.java diff --git a/jdk/src/java.base/windows/classes/sun/nio/fs/WindowsNativeDispatcher.java b/jdk/src/java.base/windows/classes/sun/nio/fs/WindowsNativeDispatcher.java index d20d12fb053..82a4bc3aecd 100644 --- a/jdk/src/java.base/windows/classes/sun/nio/fs/WindowsNativeDispatcher.java +++ b/jdk/src/java.base/windows/classes/sun/nio/fs/WindowsNativeDispatcher.java @@ -36,6 +36,17 @@ import sun.misc.Unsafe; class WindowsNativeDispatcher { private WindowsNativeDispatcher() { } + /** + * HANDLE CreateEvent( + * LPSECURITY_ATTRIBUTES lpEventAttributes, + * BOOL bManualReset, + * BOOL bInitialState, + * PCTSTR lpName + * ); + */ + static native long CreateEvent(boolean bManualReset, boolean bInitialState) + throws WindowsException; + /** * HANDLE CreateFile( * LPCTSTR lpFileName, @@ -1041,6 +1052,25 @@ class WindowsNativeDispatcher { long pOverlapped) throws WindowsException; + + /** + * CancelIo( + * HANDLE hFile + * ) + */ + static native void CancelIo(long hFile) throws WindowsException; + + /** + * GetOverlappedResult( + * HANDLE hFile, + * LPOVERLAPPED lpOverlapped, + * LPDWORD lpNumberOfBytesTransferred, + * BOOL bWait + * ); + */ + static native int GetOverlappedResult(long hFile, long lpOverlapped) + throws WindowsException; + /** * BackupRead( * HANDLE hFile, diff --git a/jdk/src/java.base/windows/classes/sun/nio/fs/WindowsWatchService.java b/jdk/src/java.base/windows/classes/sun/nio/fs/WindowsWatchService.java index 6677c874348..737c485698f 100644 --- a/jdk/src/java.base/windows/classes/sun/nio/fs/WindowsWatchService.java +++ b/jdk/src/java.base/windows/classes/sun/nio/fs/WindowsWatchService.java @@ -25,9 +25,16 @@ package sun.nio.fs; -import java.nio.file.*; import java.io.IOException; -import java.util.*; +import java.nio.file.NotDirectoryException; +import java.nio.file.Path; +import java.nio.file.StandardWatchEventKinds; +import java.nio.file.WatchEvent; +import java.nio.file.WatchKey; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; + import com.sun.nio.file.ExtendedWatchEventModifier; import sun.misc.Unsafe; @@ -42,7 +49,6 @@ class WindowsWatchService extends AbstractWatchService { private final static int WAKEUP_COMPLETION_KEY = 0; - private final Unsafe unsafe = Unsafe.getUnsafe(); // background thread to service I/O completion port private final Poller poller; @@ -82,7 +88,7 @@ class WindowsWatchService /** * Windows implementation of WatchKey. */ - private class WindowsWatchKey extends AbstractWatchKey { + private static class WindowsWatchKey extends AbstractWatchKey { // file key (used to detect existing registrations) private final FileKey fileKey; @@ -169,15 +175,9 @@ class WindowsWatchService return completionKey; } - // close directory and release buffer - void releaseResources() { - CloseHandle(handle); - buffer.cleaner().clean(); - } - - // Invalidate key by closing directory and releasing buffer + // Invalidate the key, assumes that resources have been released void invalidate() { - releaseResources(); + ((WindowsWatchService)watcher()).poller.releaseResources(this); handle = INVALID_HANDLE_VALUE; buffer = null; countAddress = 0; @@ -193,7 +193,7 @@ class WindowsWatchService public void cancel() { if (isValid()) { // delegate to poller - poller.cancel(this); + ((WindowsWatchService)watcher()).poller.cancel(this); } } } @@ -241,18 +241,25 @@ class WindowsWatchService /** * Background thread to service I/O completion port. */ - private class Poller extends AbstractPoller { + private static class Poller extends AbstractPoller { + private final static Unsafe UNSAFE = Unsafe.getUnsafe(); + /* * typedef struct _OVERLAPPED { - * DWORD Internal; - * DWORD InternalHigh; - * DWORD Offset; - * DWORD OffsetHigh; - * HANDLE hEvent; + * ULONG_PTR Internal; + * ULONG_PTR InternalHigh; + * union { + * struct { DWORD Offset; DWORD OffsetHigh; }; + * PVOID Pointer; + * }; + * HANDLE hEvent; * } OVERLAPPED; */ private static final short SIZEOF_DWORD = 4; private static final short SIZEOF_OVERLAPPED = 32; // 20 on 32-bit + private static final short OFFSETOF_HEVENT = + (UNSAFE.addressSize() == 4) ? (short) 16 : 24; + /* * typedef struct _FILE_NOTIFY_INFORMATION { @@ -276,10 +283,10 @@ class WindowsWatchService private final long port; // maps completion key to WatchKey - private final Map ck2key; + private final Map ck2key; // maps file key to WatchKey - private final Map fk2key; + private final Map fk2key; // unique completion key for each directory // native completion key capacity is 64 bits on Win64. @@ -393,8 +400,13 @@ class WindowsWatchService long overlappedAddress = bufferAddress + size - SIZEOF_OVERLAPPED; long countAddress = overlappedAddress - SIZEOF_DWORD; + // zero the overlapped structure + UNSAFE.setMemory(overlappedAddress, SIZEOF_OVERLAPPED, (byte)0); + // start async read of changes to directory try { + createAndAttachEvent(overlappedAddress); + ReadDirectoryChangesW(handle, bufferAddress, CHANGES_BUFFER_SIZE, @@ -403,6 +415,7 @@ class WindowsWatchService countAddress, overlappedAddress); } catch (WindowsException x) { + closeAttachedEvent(overlappedAddress); buffer.release(); return new IOException(x.getMessage()); } @@ -421,7 +434,7 @@ class WindowsWatchService // 2. release existing key's resources (handle/buffer) // 3. re-initialize key with new handle/buffer ck2key.remove(existing.completionKey()); - existing.releaseResources(); + releaseResources(existing); watchKey = existing.init(handle, events, watchSubtree, buffer, countAddress, overlappedAddress, completionKey); } @@ -436,6 +449,42 @@ class WindowsWatchService } } + /** + * Cancels the outstanding I/O operation on the directory + * associated with the given key and releases the associated + * resources. + */ + private void releaseResources(WindowsWatchKey key) { + try { + CancelIo(key.handle()); + GetOverlappedResult(key.handle(), key.overlappedAddress()); + } catch (WindowsException expected) { + // expected as I/O operation has been cancelled + } + CloseHandle(key.handle()); + closeAttachedEvent(key.overlappedAddress()); + key.buffer().cleaner().clean(); + } + + /** + * Creates an unnamed event and set it as the hEvent field + * in the given OVERLAPPED structure + */ + private void createAndAttachEvent(long ov) throws WindowsException { + long hEvent = CreateEvent(false, false); + UNSAFE.putAddress(ov + OFFSETOF_HEVENT, hEvent); + } + + /** + * Closes the event attached to the given OVERLAPPED structure. A + * no-op if there isn't an event attached. + */ + private void closeAttachedEvent(long ov) { + long hEvent = UNSAFE.getAddress(ov + OFFSETOF_HEVENT); + if (hEvent != 0 && hEvent != INVALID_HANDLE_VALUE) + CloseHandle(hEvent); + } + // cancel single key @Override void implCancelKey(WatchKey obj) { @@ -451,9 +500,8 @@ class WindowsWatchService @Override void implCloseAll() { // cancel all keys - for (Map.Entry entry: ck2key.entrySet()) { - entry.getValue().invalidate(); - } + ck2key.values().forEach(WindowsWatchKey::invalidate); + fk2key.clear(); ck2key.clear(); @@ -462,8 +510,7 @@ class WindowsWatchService } // Translate file change action into watch event - private WatchEvent.Kind translateActionToEvent(int action) - { + private WatchEvent.Kind translateActionToEvent(int action) { switch (action) { case FILE_ACTION_MODIFIED : return StandardWatchEventKinds.ENTRY_MODIFY; @@ -487,18 +534,18 @@ class WindowsWatchService int nextOffset; do { - int action = unsafe.getInt(address + OFFSETOF_ACTION); + int action = UNSAFE.getInt(address + OFFSETOF_ACTION); // map action to event WatchEvent.Kind kind = translateActionToEvent(action); if (key.events().contains(kind)) { // copy the name - int nameLengthInBytes = unsafe.getInt(address + OFFSETOF_FILENAMELENGTH); + int nameLengthInBytes = UNSAFE.getInt(address + OFFSETOF_FILENAMELENGTH); if ((nameLengthInBytes % 2) != 0) { - throw new AssertionError("FileNameLength.FileNameLength is not a multiple of 2"); + throw new AssertionError("FileNameLength is not a multiple of 2"); } char[] nameAsArray = new char[nameLengthInBytes/2]; - unsafe.copyMemory(null, address + OFFSETOF_FILENAME, nameAsArray, + UNSAFE.copyMemory(null, address + OFFSETOF_FILENAME, nameAsArray, Unsafe.ARRAY_CHAR_BASE_OFFSET, nameLengthInBytes); // create FileName and queue event @@ -508,7 +555,7 @@ class WindowsWatchService } // next event - nextOffset = unsafe.getInt(address + OFFSETOF_NEXTENTRYOFFSET); + nextOffset = UNSAFE.getInt(address + OFFSETOF_NEXTENTRYOFFSET); address += (long)nextOffset; } while (nextOffset != 0); } diff --git a/jdk/src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c b/jdk/src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c index 94e060fa193..8075d046763 100644 --- a/jdk/src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c +++ b/jdk/src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c @@ -195,6 +195,17 @@ Java_sun_nio_fs_WindowsNativeDispatcher_initIDs(JNIEnv* env, jclass this) } } +JNIEXPORT jlong JNICALL +Java_sun_nio_fs_WindowsNativeDispatcher_CreateEvent(JNIEnv* env, jclass this, + jboolean bManualReset, jboolean bInitialState) +{ + HANDLE hEvent = CreateEventW(NULL, bManualReset, bInitialState, NULL); + if (hEvent == NULL) { + throwWindowsException(env, GetLastError()); + } + return ptr_to_jlong(hEvent); +} + JNIEXPORT jstring JNICALL Java_sun_nio_fs_WindowsNativeDispatcher_FormatMessage(JNIEnv* env, jclass this, jint errorCode) { WCHAR message[255]; @@ -1229,6 +1240,31 @@ Java_sun_nio_fs_WindowsNativeDispatcher_PostQueuedCompletionStatus(JNIEnv* env, } } +JNIEXPORT void JNICALL +Java_sun_nio_fs_WindowsNativeDispatcher_CancelIo(JNIEnv* env, jclass this, jlong hFile) { + if (CancelIo((HANDLE)jlong_to_ptr(hFile)) == 0) { + throwWindowsException(env, GetLastError()); + } +} + +JNIEXPORT jint JNICALL +Java_sun_nio_fs_WindowsNativeDispatcher_GetOverlappedResult(JNIEnv *env, jclass this, + jlong hFile, jlong lpOverlapped) +{ + BOOL res; + DWORD bytesTransferred = -1; + + res = GetOverlappedResult((HANDLE)jlong_to_ptr(hFile), + (LPOVERLAPPED)jlong_to_ptr(lpOverlapped), + &bytesTransferred, + TRUE); + if (res == 0) { + throwWindowsException(env, GetLastError()); + } + + return (jint)bytesTransferred; +} + JNIEXPORT void JNICALL Java_sun_nio_fs_WindowsNativeDispatcher_ReadDirectoryChangesW(JNIEnv* env, jclass this, jlong hDirectory, jlong bufferAddress, jint bufferLength, jboolean watchSubTree, jint filter, @@ -1236,17 +1272,7 @@ Java_sun_nio_fs_WindowsNativeDispatcher_ReadDirectoryChangesW(JNIEnv* env, jclas { BOOL res; BOOL subtree = (watchSubTree == JNI_TRUE) ? TRUE : FALSE; - - /* Any unused members of [OVERLAPPED] structure should always be initialized to zero - before the structure is used in a function call. - Otherwise, the function may fail and return ERROR_INVALID_PARAMETER. - http://msdn.microsoft.com/en-us/library/windows/desktop/ms684342%28v=vs.85%29.aspx - - The [Offset] and [OffsetHigh] members of this structure are not used. - http://msdn.microsoft.com/en-us/library/windows/desktop/aa365465%28v=vs.85%29.aspx - - [hEvent] should be zero, other fields are the return values. */ - ZeroMemory((LPOVERLAPPED)jlong_to_ptr(pOverlapped), sizeof(OVERLAPPED)); + LPOVERLAPPED ov = (LPOVERLAPPED)jlong_to_ptr(pOverlapped); res = ReadDirectoryChangesW((HANDLE)jlong_to_ptr(hDirectory), (LPVOID)jlong_to_ptr(bufferAddress), diff --git a/jdk/test/java/nio/file/WatchService/LotsOfCancels.java b/jdk/test/java/nio/file/WatchService/LotsOfCancels.java new file mode 100644 index 00000000000..cb7f9eff9af --- /dev/null +++ b/jdk/test/java/nio/file/WatchService/LotsOfCancels.java @@ -0,0 +1,119 @@ +/* + * Copyright (c) 2014, 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. + */ + +/* @test + * @bug 8029516 + * @summary Bash on WatchKey.cancel with a view to causing a crash when + * an outstanding I/O operation on directory completes after the + * directory has been closed + */ + +import java.nio.file.ClosedWatchServiceException; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.WatchKey; +import java.nio.file.WatchService; +import static java.nio.file.StandardWatchEventKinds.*; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; + +public class LotsOfCancels { + + // set to true for any exceptions + static volatile boolean failed; + + public static void main(String[] args) throws Exception { + + // create a bunch of directories. Create two tasks for each directory, + // one to bash on cancel, the other to poll the events + ExecutorService pool = Executors.newCachedThreadPool(); + try { + Path top = Files.createTempDirectory("work"); + top.toFile().deleteOnExit(); + for (int i=1; i<=16; i++) { + Path dir = Files.createDirectory(top.resolve("dir-" + i)); + WatchService watcher = FileSystems.getDefault().newWatchService(); + pool.submit(() -> handle(dir, watcher)); + pool.submit(() -> poll(watcher)); + } + } finally { + pool.shutdown(); + } + + // give thread pool lots of time to terminate + if (!pool.awaitTermination(5L, TimeUnit.MINUTES)) + throw new RuntimeException("Thread pool did not terminate"); + + if (failed) + throw new RuntimeException("Test failed, see log for details"); + } + + /** + * Stress the given WatchService, specifically the cancel method, in + * the given directory. Closes the WatchService when done. + */ + static void handle(Path dir, WatchService watcher) { + try { + try { + Path file = dir.resolve("anyfile"); + for (int i=0; i<2000; i++) { + WatchKey key = dir.register(watcher, ENTRY_CREATE, ENTRY_DELETE); + Files.createFile(file); + Files.delete(file); + key.cancel(); + } + } finally { + watcher.close(); + } + } catch (Exception e) { + e.printStackTrace(); + failed = true; + } + } + + /** + * Polls the given WatchService in a tight loop. This keeps the event + * queue drained, it also hogs a CPU core which seems necessary to + * tickle the original bug. + */ + static void poll(WatchService watcher) { + try { + for (;;) { + WatchKey key = watcher.take(); + if (key != null) { + key.pollEvents(); + key.reset(); + } + } + } catch (ClosedWatchServiceException expected) { + // nothing to do + } catch (Exception e) { + e.printStackTrace(); + failed = true; + } + } + +} +