8216472: (se) Stack overflow during selection operation leads to crash (win)

Reviewed-by: alanb

src/java.base/windows/classes/sun/nio/ch/WindowsSelectorImpl.java

@@ -38,6 +38,7 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.function.Consumer;
+import jdk.internal.misc.Unsafe;
 
 /**
  * A multi-threaded implementation of Selector for Windows.
@@ -47,12 +48,26 @@ import java.util.function.Consumer;
  */
 
 class WindowsSelectorImpl extends SelectorImpl {
+    private static final Unsafe unsafe = Unsafe.getUnsafe();
+    private static int addressSize = unsafe.addressSize();
+
+    private static int dependsArch(int value32, int value64) {
+        return (addressSize == 4) ? value32 : value64;
+    }
+
     // Initial capacity of the poll array
     private final int INIT_CAP = 8;
     // Maximum number of sockets for select().
     // Should be INIT_CAP times a power of 2
     private static final int MAX_SELECTABLE_FDS = 1024;
 
+    // Size of FD_SET struct to allocate a buffer for it in SubSelector,
+    // aligned to 8 bytes on 64-bit:
+    // struct { unsigned int fd_count; SOCKET fd_array[MAX_SELECTABLE_FDS]; }.
+    private static final long SIZEOF_FD_SET = dependsArch(
+            4 + MAX_SELECTABLE_FDS * 4,      // SOCKET = unsigned int
+            4 + MAX_SELECTABLE_FDS * 8 + 4); // SOCKET = unsigned __int64
+
     // The list of SelectableChannels serviced by this Selector. Every mod
     // MAX_SELECTABLE_FDS entry is bogus, to align this array with the poll
     // array,  where the corresponding entry is occupied by the wakeupSocket
@@ -326,6 +341,9 @@ class WindowsSelectorImpl extends SelectorImpl {
         private final int[] readFds = new int [MAX_SELECTABLE_FDS + 1];
         private final int[] writeFds = new int [MAX_SELECTABLE_FDS + 1];
         private final int[] exceptFds = new int [MAX_SELECTABLE_FDS + 1];
+        // Buffer for readfds, writefds and exceptfds structs that are passed
+        // to native select().
+        private final long fdsBuffer = unsafe.allocateMemory(SIZEOF_FD_SET * 3);
 
         private SubSelector() {
             this.pollArrayIndex = 0; // main thread
@@ -338,7 +356,7 @@ class WindowsSelectorImpl extends SelectorImpl {
         private int poll() throws IOException{ // poll for the main thread
             return poll0(pollWrapper.pollArrayAddress,
                          Math.min(totalChannels, MAX_SELECTABLE_FDS),
-                         readFds, writeFds, exceptFds, timeout);
+                         readFds, writeFds, exceptFds, timeout, fdsBuffer);
         }
 
         private int poll(int index) throws IOException {
@@ -347,11 +365,11 @@ class WindowsSelectorImpl extends SelectorImpl {
                      (pollArrayIndex * PollArrayWrapper.SIZE_POLLFD),
                      Math.min(MAX_SELECTABLE_FDS,
                              totalChannels - (index + 1) * MAX_SELECTABLE_FDS),
-                     readFds, writeFds, exceptFds, timeout);
+                     readFds, writeFds, exceptFds, timeout, fdsBuffer);
         }
 
         private native int poll0(long pollAddress, int numfds,
-             int[] readFds, int[] writeFds, int[] exceptFds, long timeout);
+             int[] readFds, int[] writeFds, int[] exceptFds, long timeout, long fdsBuffer);
 
         private int processSelectedKeys(long updateCount, Consumer<SelectionKey> action) {
             int numKeysUpdated = 0;
@@ -415,6 +433,10 @@ class WindowsSelectorImpl extends SelectorImpl {
             }
             return numKeysUpdated;
         }
+
+        private void freeFDSetBuffer() {
+            unsafe.freeMemory(fdsBuffer);
+        }
     }
 
     // Represents a helper thread used for select.
@@ -441,8 +463,10 @@ class WindowsSelectorImpl extends SelectorImpl {
             while (true) { // poll loop
                 // wait for the start of poll. If this thread has become
                 // redundant, then exit.
-                if (startLock.waitForStart(this))
+                if (startLock.waitForStart(this)) {
+                    subSelector.freeFDSetBuffer();
                     return;
+                }
                 // call poll()
                 try {
                     subSelector.poll(index);
@@ -533,6 +557,7 @@ class WindowsSelectorImpl extends SelectorImpl {
         for (SelectThread t: threads)
              t.makeZombie();
         startLock.startThreads();
+        subSelector.freeFDSetBuffer();
     }
 
     @Override

src/java.base/windows/native/libnio/ch/WindowsSelectorImpl.c

@@ -39,6 +39,7 @@
 #include "jvm.h"
 #include "jni.h"
 #include "jni_util.h"
+#include "nio.h"
 #include "sun_nio_ch_WindowsSelectorImpl.h"
 #include "sun_nio_ch_PollArrayWrapper.h"
 
@@ -56,12 +57,14 @@ JNIEXPORT jint JNICALL
 Java_sun_nio_ch_WindowsSelectorImpl_00024SubSelector_poll0(JNIEnv *env, jobject this,
                                    jlong pollAddress, jint numfds,
                                    jintArray returnReadFds, jintArray returnWriteFds,
-                                   jintArray returnExceptFds, jlong timeout)
+                                   jintArray returnExceptFds, jlong timeout, jlong fdsBuffer)
 {
     DWORD result = 0;
     pollfd *fds = (pollfd *) pollAddress;
     int i;
-    FD_SET readfds, writefds, exceptfds;
+    FD_SET *readfds = (FD_SET *) jlong_to_ptr(fdsBuffer);
+    FD_SET *writefds = (FD_SET *) jlong_to_ptr(fdsBuffer + sizeof(FD_SET));
+    FD_SET *exceptfds = (FD_SET *) jlong_to_ptr(fdsBuffer + sizeof(FD_SET) * 2);
     struct timeval timevalue, *tv;
     static struct timeval zerotime = {0, 0};
     int read_count = 0, write_count = 0, except_count = 0;
@@ -93,103 +96,61 @@ Java_sun_nio_ch_WindowsSelectorImpl_00024SubSelector_poll0(JNIEnv *env, jobject
     /* Set FD_SET structures required for select */
     for (i = 0; i < numfds; i++) {
         if (fds[i].events & POLLIN) {
-           readfds.fd_array[read_count] = fds[i].fd;
+           readfds->fd_array[read_count] = fds[i].fd;
            read_count++;
         }
         if (fds[i].events & (POLLOUT | POLLCONN))
         {
-           writefds.fd_array[write_count] = fds[i].fd;
+           writefds->fd_array[write_count] = fds[i].fd;
            write_count++;
         }
-        exceptfds.fd_array[except_count] = fds[i].fd;
+        exceptfds->fd_array[except_count] = fds[i].fd;
         except_count++;
     }
 
-    readfds.fd_count = read_count;
+    readfds->fd_count = read_count;
-    writefds.fd_count = write_count;
+    writefds->fd_count = write_count;
-    exceptfds.fd_count = except_count;
+    exceptfds->fd_count = except_count;
 
     /* Call select */
-    if ((result = select(0 , &readfds, &writefds, &exceptfds, tv))
+    if ((result = select(0 , readfds, writefds, exceptfds, tv))
                                                              == SOCKET_ERROR) {
-        /* Bad error - this should not happen frequently */
+        JNU_ThrowIOExceptionWithLastError(env, "Select failed");
-        /* Iterate over sockets and call select() on each separately */
+        return IOS_THROWN;
-        FD_SET errreadfds, errwritefds, errexceptfds;
-        readfds.fd_count = 0;
-        writefds.fd_count = 0;
-        exceptfds.fd_count = 0;
-        for (i = 0; i < numfds; i++) {
-            /* prepare select structures for the i-th socket */
-            errreadfds.fd_count = 0;
-            errwritefds.fd_count = 0;
-            if (fds[i].events & POLLIN) {
-               errreadfds.fd_array[0] = fds[i].fd;
-               errreadfds.fd_count = 1;
-            }
-            if (fds[i].events & (POLLOUT | POLLCONN))
-            {
-                errwritefds.fd_array[0] = fds[i].fd;
-                errwritefds.fd_count = 1;
-            }
-            errexceptfds.fd_array[0] = fds[i].fd;
-            errexceptfds.fd_count = 1;
-
-            /* call select on the i-th socket */
-            if (select(0, &errreadfds, &errwritefds, &errexceptfds, &zerotime)
-                                                             == SOCKET_ERROR) {
-                /* This socket causes an error. Add it to exceptfds set */
-                exceptfds.fd_array[exceptfds.fd_count] = fds[i].fd;
-                exceptfds.fd_count++;
-            } else {
-                /* This socket does not cause an error. Process result */
-                if (errreadfds.fd_count == 1) {
-                    readfds.fd_array[readfds.fd_count] = fds[i].fd;
-                    readfds.fd_count++;
-                }
-                if (errwritefds.fd_count == 1) {
-                    writefds.fd_array[writefds.fd_count] = fds[i].fd;
-                    writefds.fd_count++;
-                }
-                if (errexceptfds.fd_count == 1) {
-                    exceptfds.fd_array[exceptfds.fd_count] = fds[i].fd;
-                    exceptfds.fd_count++;
-                }
-            }
-        }
     }
 
     /* Return selected sockets. */
     /* Each Java array consists of sockets count followed by sockets list */
 
 #ifdef _WIN64
-    resultbuf[0] = readfds.fd_count;
+    resultbuf[0] = readfds->fd_count;
-    for (i = 0; i < (int)readfds.fd_count; i++) {
+    for (i = 0; i < (int)readfds->fd_count; i++) {
-        resultbuf[i + 1] = (int)readfds.fd_array[i];
+        resultbuf[i + 1] = (int)readfds->fd_array[i];
     }
     (*env)->SetIntArrayRegion(env, returnReadFds, 0,
-                              readfds.fd_count + 1, resultbuf);
+                              readfds->fd_count + 1, resultbuf);
 
-    resultbuf[0] = writefds.fd_count;
+    resultbuf[0] = writefds->fd_count;
-    for (i = 0; i < (int)writefds.fd_count; i++) {
+    for (i = 0; i < (int)writefds->fd_count; i++) {
-        resultbuf[i + 1] = (int)writefds.fd_array[i];
+        resultbuf[i + 1] = (int)writefds->fd_array[i];
     }
     (*env)->SetIntArrayRegion(env, returnWriteFds, 0,
-                              writefds.fd_count + 1, resultbuf);
+                              writefds->fd_count + 1, resultbuf);
 
-    resultbuf[0] = exceptfds.fd_count;
+    resultbuf[0] = exceptfds->fd_count;
-    for (i = 0; i < (int)exceptfds.fd_count; i++) {
+    for (i = 0; i < (int)exceptfds->fd_count; i++) {
-        resultbuf[i + 1] = (int)exceptfds.fd_array[i];
+        resultbuf[i + 1] = (int)exceptfds->fd_array[i];
     }
     (*env)->SetIntArrayRegion(env, returnExceptFds, 0,
-                              exceptfds.fd_count + 1, resultbuf);
+                              exceptfds->fd_count + 1, resultbuf);
 #else
     (*env)->SetIntArrayRegion(env, returnReadFds, 0,
-                              readfds.fd_count + 1, (jint *)&readfds);
+                              readfds->fd_count + 1, (jint *)readfds);
 
     (*env)->SetIntArrayRegion(env, returnWriteFds, 0,
-                              writefds.fd_count + 1, (jint *)&writefds);
+                              writefds->fd_count + 1, (jint *)writefds);
     (*env)->SetIntArrayRegion(env, returnExceptFds, 0,
-                              exceptfds.fd_count + 1, (jint *)&exceptfds);
+                              exceptfds->fd_count + 1, (jint *)exceptfds);
 #endif
     return 0;
 }

test/jdk/java/nio/channels/Selector/StackOverflowTest.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright (c) 2019, Red Hat, Inc. and/or its affiliates.
+ * 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 8216472
+ * @summary native call in WindowsSelectorImpl.SubSelector.poll can use
+ *     more stack space than available in a shadow zone, this can cause
+ *     a crash if selector is called from a deep recursive java call
+ * @requires (os.family == "windows")
+ */
+
+import java.nio.channels.Selector;
+
+public class StackOverflowTest {
+
+    public static void main(String[] args) throws Exception {
+        try (var sel = Selector.open()) {
+            recursiveSelect(sel);
+        } catch (StackOverflowError e) {
+            // ignore SOE from java calls
+        }
+    }
+
+    static void recursiveSelect(Selector sel) throws Exception {
+        sel.selectNow();
+        recursiveSelect(sel);
+    }
+}