From 8d16374b02e6bc5f330e3851de016a92be3fba40 Mon Sep 17 00:00:00 2001
From: Nick Banks <nibanks@microsoft.com>
Date: Fri, 31 Jan 2020 11:32:27 -0800
Subject: [PATCH] Don't Leak Internal (server) Connections on Shutdown (#82)

This fixes an existing bug where a connection can be leaked if the library is cleaned up while there are any outstanding internal connections. The fix is to have the worker shut them down if there is no external owner when the worker's thread is stopped.

The PR also disabled event validation tests on schannel, since they require the connection to succeed (which doesn't work on Azure Pipelines, because Schannel TLS 1.3 isn't supported yet).
---
 azure-pipelines.yml |  2 +-
 core/connection.c   |  6 ------
 core/connection.h   |  6 ++++++
 core/worker.c       | 10 +++++++++-
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/azure-pipelines.yml b/azure-pipelines.yml
index 52856cb35e98..3a0c17ada194 100644
--- a/azure-pipelines.yml
+++ b/azure-pipelines.yml
@@ -47,7 +47,7 @@ strategy:
       platform: 'windows'
       config: 'schannel'
       buildArgs: '-Tls schannel'
-      testCmd: '.\.azure\test_windows.cmd ParameterValidation.*'
+      testCmd: '.\.azure\test_windows.cmd ParameterValidation.*:-*Events'
 
 pool:
   vmImage: '$(platform)-latest'
diff --git a/core/connection.c b/core/connection.c
index 30ad1f7d4661..1c7247ac9055 100644
--- a/core/connection.c
+++ b/core/connection.c
@@ -43,12 +43,6 @@ QuicConnInitializeCrypto(
     _In_ QUIC_CONNECTION* Connection
     );
 
-_IRQL_requires_max_(PASSIVE_LEVEL)
-void
-QuicConnOnShutdownComplete(
-    _In_ QUIC_CONNECTION* Connection
-    );
-
 _IRQL_requires_max_(DISPATCH_LEVEL)
 __drv_allocatesMem(Mem)
 _Must_inspect_result_
diff --git a/core/connection.h b/core/connection.h
index 3d106cd0bb40..ab7011581e31 100644
--- a/core/connection.h
+++ b/core/connection.h
@@ -802,6 +802,12 @@ QuicConnCloseHandle(
     _In_ QUIC_CONNECTION* Connection
     );
 
+_IRQL_requires_max_(PASSIVE_LEVEL)
+void
+QuicConnOnShutdownComplete(
+    _In_ QUIC_CONNECTION* Connection
+    );
+
 #if QUIC_TEST_MODE
 _IRQL_requires_max_(DISPATCH_LEVEL)
 inline
diff --git a/core/worker.c b/core/worker.c
index 42ebc1a629fc..9236e1b6758c 100644
--- a/core/worker.c
+++ b/core/worker.c
@@ -581,6 +581,14 @@ QUIC_THREAD_CALLBACK(QuicWorkerThread, Context)
         QUIC_CONNECTION* Connection =
             QUIC_CONTAINING_RECORD(
                 QuicListRemoveHead(&Worker->Connections), QUIC_CONNECTION, WorkerLink);
+        if (!Connection->State.ExternalOwner) {
+            //
+            // If there is no external owner, shut down the connection so that
+            // it's not leaked.
+            //
+            QuicTraceLogConnVerbose(AbandonOnLibShutdown, Connection, "Abandoning on shutdown");
+            QuicConnOnShutdownComplete(Connection);
+        }
         QuicConnRelease(Connection, QUIC_CONN_REF_WORKER);
     }
 
@@ -705,4 +713,4 @@ QuicWorkerPoolGetLeastLoadedWorker(
 
     WorkerPool->LastWorker = MinQueueDelayWorker;
     return MinQueueDelayWorker;
-}
\ No newline at end of file
+}