8029516: (fs) WatchKey cancel unreliable on Windows

Reviewed-by: chegar

src/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,

src/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<Integer,WindowsWatchKey> ck2key;
+        private final Map<Integer, WindowsWatchKey> ck2key;
 
         // maps file key to WatchKey
-        private final Map<FileKey,WindowsWatchKey> fk2key;
+        private final Map<FileKey, WindowsWatchKey> 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<Integer, WindowsWatchKey> 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);
         }

src/windows/native/sun/nio/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),

test/java/nio/file/WatchService/LotsOfCancels.java

+/*
+ * 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;
+        }
+    }
+
+}
+