win,tcp: avoid reinserting a pending request (libuv#2688)

This fix avoids inserting a duplicate pending request in the case where `WSARecv()` returns an error (e.g. when a connection has been terminated by its peer) when `uv_read_start()` is called in a read callback. Fixes: libuv#2687 PR-URL: libuv#2688 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Jameson Nash <vtjnash@gmail.com>
escherstair · Jul 28, 2020 · f779fd4 · f779fd4
1 parent ca10e36
commit f779fd4
Show file tree

Hide file tree

Showing 5 changed files with 144 additions and 5 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -478,6 +478,7 @@ if(LIBUV_BUILD_TESTS)
        test/test-tcp-oob.c
        test/test-tcp-open.c
        test/test-tcp-read-stop.c
+       test/test-tcp-read-stop-start.c
        test/test-tcp-shutdown-after-write.c
        test/test-tcp-try-write.c
        test/test-tcp-try-write-error.c

diff --git a/Makefile.am b/Makefile.am
@@ -262,6 +262,7 @@ test_run_tests_SOURCES = test/blackhole-server.c \
                          test/test-tcp-flags.c \
                          test/test-tcp-open.c \
                          test/test-tcp-read-stop.c \
+                         test/test-tcp-read-stop-start.c \
                          test/test-tcp-shutdown-after-write.c \
                          test/test-tcp-unexpected-read.c \
                          test/test-tcp-oob.c \

diff --git a/src/win/tcp.c b/src/win/tcp.c
@@ -523,16 +523,15 @@ static void uv_tcp_queue_read(uv_loop_t* loop, uv_tcp_t* handle) {
                    &req->u.io.overlapped,
                    NULL);
 
+  handle->flags |= UV_HANDLE_READ_PENDING;
+  handle->reqs_pending++;
+
   if (UV_SUCCEEDED_WITHOUT_IOCP(result == 0)) {
     /* Process the req without IOCP. */
-    handle->flags |= UV_HANDLE_READ_PENDING;
     req->u.io.overlapped.InternalHigh = bytes;
-    handle->reqs_pending++;
     uv_insert_pending_req(loop, (uv_req_t*)req);
   } else if (UV_SUCCEEDED_WITH_IOCP(result == 0)) {
     /* The req will be processed with IOCP. */
-    handle->flags |= UV_HANDLE_READ_PENDING;
-    handle->reqs_pending++;
     if (handle->flags & UV_HANDLE_EMULATE_IOCP &&
         req->wait_handle == INVALID_HANDLE_VALUE &&
         !RegisterWaitForSingleObject(&req->wait_handle,
@@ -545,7 +544,6 @@ static void uv_tcp_queue_read(uv_loop_t* loop, uv_tcp_t* handle) {
     /* Make this req pending reporting an error. */
     SET_REQ_ERROR(req, WSAGetLastError());
     uv_insert_pending_req(loop, (uv_req_t*)req);
-    handle->reqs_pending++;
   }
 }
 

diff --git a/test/test-list.h b/test/test-list.h
@@ -145,6 +145,7 @@ TEST_DECLARE   (tcp_flags)
 TEST_DECLARE   (tcp_write_to_half_open_connection)
 TEST_DECLARE   (tcp_unexpected_read)
 TEST_DECLARE   (tcp_read_stop)
+TEST_DECLARE   (tcp_read_stop_start)
 TEST_DECLARE   (tcp_bind6_error_addrinuse)
 TEST_DECLARE   (tcp_bind6_error_addrnotavail)
 TEST_DECLARE   (tcp_bind6_error_fault)
@@ -697,6 +698,8 @@ TASK_LIST_START
   TEST_ENTRY  (tcp_read_stop)
   TEST_HELPER (tcp_read_stop, tcp4_echo_server)
 
+  TEST_ENTRY  (tcp_read_stop_start)
+
   TEST_ENTRY  (tcp_bind6_error_addrinuse)
   TEST_ENTRY  (tcp_bind6_error_addrnotavail)
   TEST_ENTRY  (tcp_bind6_error_fault)

diff --git a/test/test-tcp-read-stop-start.c b/test/test-tcp-read-stop-start.c
@@ -0,0 +1,136 @@
+/* Copyright libuv project contributors. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "uv.h"
+#include "task.h"
+
+static uv_tcp_t server;
+static uv_tcp_t connection;
+static int read_cb_called = 0;
+
+static uv_tcp_t client;
+static uv_connect_t connect_req;
+
+
+static void on_read2(uv_stream_t* stream, ssize_t nread, const uv_buf_t* buf);
+
+static void on_write_close_immediately(uv_write_t* req, int status) {
+  ASSERT(0 == status);
+
+  uv_close((uv_handle_t*)req->handle, NULL); /* Close immediately */
+  free(req);
+}
+
+static void on_write(uv_write_t* req, int status) {
+  ASSERT(0 == status);
+
+  free(req);
+}
+
+static void do_write(uv_stream_t* stream, uv_write_cb cb) {
+  uv_write_t* req = malloc(sizeof(*req));
+  uv_buf_t buf;
+  buf.base = "1234578";
+  buf.len = 8;
+  ASSERT(0 == uv_write(req, stream, &buf, 1, cb));
+}
+
+static void on_alloc(uv_handle_t* handle,
+                     size_t suggested_size,
+                     uv_buf_t* buf) {
+  static char slab[65536];
+  buf->base = slab;
+  buf->len = sizeof(slab);
+}
+
+static void on_read1(uv_stream_t* stream, ssize_t nread, const uv_buf_t* buf) {
+  ASSERT(nread >= 0);
+
+  /* Do write on a half open connection to force WSAECONNABORTED (on Windows)
+   * in the subsequent uv_read_start()
+   */
+  do_write(stream, on_write);
+
+  ASSERT(0 == uv_read_stop(stream));
+
+  ASSERT(0 == uv_read_start(stream, on_alloc, on_read2));
+
+  read_cb_called++;
+}
+
+static void on_read2(uv_stream_t* stream, ssize_t nread, const uv_buf_t* buf) {
+  ASSERT(nread < 0);
+
+  uv_close((uv_handle_t*)stream, NULL);
+  uv_close((uv_handle_t*)&server, NULL);
+
+  read_cb_called++;
+}
+
+static void on_connection(uv_stream_t* server, int status) {
+  ASSERT(0 == status);
+
+  ASSERT(0 == uv_tcp_init(server->loop, &connection));
+
+  ASSERT(0 == uv_accept(server, (uv_stream_t* )&connection));
+
+  ASSERT(0 == uv_read_start((uv_stream_t*)&connection, on_alloc, on_read1));
+}
+
+static void on_connect(uv_connect_t* req, int status) {
+  ASSERT(0 == status);
+
+  do_write((uv_stream_t*)&client, on_write_close_immediately);
+}
+
+TEST_IMPL(tcp_read_stop_start) {
+  uv_loop_t* loop = uv_default_loop();
+
+  { /* Server */
+    struct sockaddr_in addr;
+
+    ASSERT(0 == uv_ip4_addr("0.0.0.0", TEST_PORT, &addr));
+
+    ASSERT(0 == uv_tcp_init(loop, &server));
+
+    ASSERT(0 == uv_tcp_bind(&server, (struct sockaddr*) & addr, 0));
+
+    ASSERT(0 == uv_listen((uv_stream_t*)&server, 10, on_connection));
+  }
+
+  { /* Client */
+    struct sockaddr_in addr;
+
+    ASSERT(0 == uv_ip4_addr("127.0.0.1", TEST_PORT, &addr));
+
+    ASSERT(0 == uv_tcp_init(loop, &client));
+
+    ASSERT(0 == uv_tcp_connect(&connect_req, &client,
+                               (const struct sockaddr*) & addr, on_connect));
+  }
+
+  ASSERT(0 == uv_run(loop, UV_RUN_DEFAULT));
+
+  ASSERT(read_cb_called >= 2);
+
+  MAKE_VALGRIND_HAPPY();
+  return 0;
+}