Clear out rebound paths upon path exhaustion (microsoft#1783)

* Clear out rebound paths upon path exhaustion

Currently we have a hard limit of 4 paths for a connection. Rebinding can possibly fill up these paths, causing the connection to be dropped. As a solution, if we detect we're out of paths, we can see if any of the non active paths are on the same local address, and the same remote IP address, but just a different port, and if so we can discard the old paths, as if theyre used again they'll just show up as another rebind.

Also adds a test that rebinds 50 times

* Test and code cleanup

* Fix review comments

* Avoid address copy

* Fix path test on linux, only consider non retired CID's as unused

* Fix mtu

* Increase wait timeout

* Wait for server to respond

src/core/connection.c

@@ -967,7 +967,7 @@ QuicConnGetUnusedDestCid(
                 Entry,
                 QUIC_CID_LIST_ENTRY,
                 Link);
-        if (!DestCid->CID.UsedLocally) {
+        if (!DestCid->CID.UsedLocally && !DestCid->CID.Retired) {
             return DestCid;
         }
     }

src/core/path.c

@@ -56,7 +56,6 @@ QuicPathRemove(
 
 #if DEBUG
     if (Path->DestCid) {
-        CXPLAT_DBG_ASSERT(Path->DestCid->AssignedPath != NULL);
         QUIC_CID_CLEAR_PATH(Path->DestCid);
     }
 #endif
@@ -196,9 +195,27 @@ QuicConnGetPathForDatagram(
 
     if (Connection->PathsCount == QUIC_MAX_PATH_COUNT) {
         //
-        // Already tracking the maximum number of paths.
+        // See if any old paths share the same remote address, and is just a rebind.
+        // If so, remove the old paths.
+        // NB: Traversing the array backwards is simpler and more efficient here due
+        // to the array shifting that happens in QuicPathRemove.
         //
-        return NULL;
+        for (uint8_t i = Connection->PathsCount - 1; i > 0; i--) {
+            if (!Connection->Paths[i].IsActive
+                && QuicAddrGetFamily(&Datagram->Tuple->RemoteAddress) == QuicAddrGetFamily(&Connection->Paths[i].RemoteAddress)
+                && QuicAddrCompareIp(&Datagram->Tuple->RemoteAddress, &Connection->Paths[i].RemoteAddress)
+                && QuicAddrCompare(&Datagram->Tuple->LocalAddress, &Connection->Paths[i].LocalAddress)) {
+                QuicPathRemove(Connection, i);
+            }
+        }
+
+        if (Connection->PathsCount == QUIC_MAX_PATH_COUNT) {
+            //
+            // Already tracking the maximum number of paths, and can't free
+            // any more.
+            //
+            return NULL;
+        }
     }
 
     if (Connection->PathsCount > 1) {

src/inc/msquic.hpp

@@ -379,6 +379,7 @@ class MsQuicSettings : public QUIC_SETTINGS {
     MsQuicSettings& SetMinimumMtu(uint16_t Mtu) { MinimumMtu = Mtu; IsSet.MinimumMtu = TRUE; return *this; }
     MsQuicSettings& SetMtuDiscoverySearchCompleteTimeoutUs(uint64_t Time) { MtuDiscoverySearchCompleteTimeoutUs = Time; IsSet.MtuDiscoverySearchCompleteTimeoutUs = TRUE; return *this; }
     MsQuicSettings& SetMtuDiscoveryMissingProbeCount(uint8_t Count) { MtuDiscoveryMissingProbeCount = Count; IsSet.MtuDiscoveryMissingProbeCount = TRUE; return *this; }
+    MsQuicSettings& SetKeepAlive(uint32_t Time) { KeepAliveIntervalMs = Time; IsSet.KeepAliveIntervalMs = TRUE; return *this; }
 
     QUIC_STATUS
     SetGlobal() const noexcept {
@@ -779,6 +780,17 @@ struct MsQuicConnection {
                 &Addr.SockAddr);
     }
 
+    QUIC_STATUS
+    GetRemoteAddr(_Out_ QuicAddr& Addr) {
+        uint32_t Size = sizeof(Addr.SockAddr);
+        return
+            GetParam(
+                QUIC_PARAM_LEVEL_CONNECTION,
+                QUIC_PARAM_CONN_REMOTE_ADDRESS,
+                &Size,
+                &Addr.SockAddr);
+    }
+
     QUIC_STATUS
     SetLocalAddr(_In_ const QuicAddr& Addr) noexcept {
         return

src/test/MsQuicTests.h

@@ -72,6 +72,14 @@ QuicTestMtuDiscovery(
     _In_ BOOLEAN RaiseMinimumMtu
     );
 
+//
+// Path tests
+//
+void
+QuicTestLocalPathChanges(
+    _In_ int Family
+    );
+
 //
 // Handshake Tests
 //
@@ -876,4 +884,8 @@ typedef struct {
 #define IOCTL_QUIC_RUN_STREAM_PRIORITY \
     QUIC_CTL_CODE(72, METHOD_BUFFERED, FILE_WRITE_DATA)
 
-#define QUIC_MAX_IOCTL_FUNC_CODE 72
+#define IOCTL_QUIC_RUN_CLIENT_LOCAL_PATH_CHANGES \
+    QUIC_CTL_CODE(73, METHOD_BUFFERED, FILE_WRITE_DATA)
+    // int - Family
+
+#define QUIC_MAX_IOCTL_FUNC_CODE 73

src/test/bin/quic_gtest.cpp

@@ -338,6 +338,15 @@ TEST(Basic, CreateConnection) {
 
 #ifdef QUIC_TEST_DATAPATH_HOOKS_ENABLED
 
+TEST_P(WithFamilyArgs, LocalPathChanges) {
+    TestLoggerT<ParamType> Logger("QuicTestLocalPathChanges", GetParam());
+    if (TestingKernelMode) {
+        ASSERT_TRUE(DriverClient.Run(IOCTL_QUIC_RUN_CLIENT_LOCAL_PATH_CHANGES, GetParam().Family));
+    } else {
+        QuicTestLocalPathChanges(GetParam().Family);
+    }
+}
+
 TEST(Mtu, Settings) {
     TestLogger Logger("QuicTestMtuSettings");
     if (TestingKernelMode) {

src/test/bin/winkernel/control.cpp

@@ -443,6 +443,7 @@ size_t QUIC_IOCTL_BUFFER_SIZES[] =
     sizeof(INT32),
     0,
     0,
+    sizeof(INT32),
 };
 
 CXPLAT_STATIC_ASSERT(
@@ -1083,6 +1084,11 @@ QuicTestCtlEvtIoDeviceControl(
         QuicTestCtlRun(QuicTestStreamPriority());
         break;
 
+    case IOCTL_QUIC_RUN_CLIENT_LOCAL_PATH_CHANGES:
+        CXPLAT_FRE_ASSERT(Params != nullptr);
+        QuicTestCtlRun(QuicTestLocalPathChanges(Params->Family));
+        break;
+
     default:
         Status = STATUS_NOT_IMPLEMENTED;
         break;

src/test/lib/CMakeLists.txt

@@ -10,6 +10,7 @@ set(SOURCES
     EventTest.cpp
     HandshakeTest.cpp
     MtuTest.cpp
+    PathTest.cpp
     QuicDrill.cpp
     TestConnection.cpp
     TestListener.cpp

src/test/lib/PathTest.cpp

@@ -0,0 +1,114 @@
+/*++
+
+    Copyright (c) Microsoft Corporation.
+    Licensed under the MIT License.
+
+Abstract:
+
+    MsQuic Path Unittest
+
+--*/
+
+#include "precomp.h"
+#ifdef QUIC_CLOG
+#include "PathTest.cpp.clog.h"
+#endif
+
+struct PathTestContext {
+    CxPlatEvent HandshakeCompleteEvent;
+    CxPlatEvent ShutdownEvent;
+    MsQuicConnection* Connection {nullptr};
+    CxPlatEvent PeerAddrChangedEvent;
+
+    static QUIC_STATUS ConnCallback(_In_ MsQuicConnection* Conn, _In_opt_ void* Context, _Inout_ QUIC_CONNECTION_EVENT* Event) {
+        PathTestContext* Ctx = static_cast<PathTestContext*>(Context);
+        Ctx->Connection = Conn;
+        if (Event->Type == QUIC_CONNECTION_EVENT_SHUTDOWN_COMPLETE) {
+            Ctx->Connection = nullptr;
+            Ctx->PeerAddrChangedEvent.Set();
+            Ctx->ShutdownEvent.Set();
+            Ctx->HandshakeCompleteEvent.Set();
+        } else if (Event->Type == QUIC_CONNECTION_EVENT_CONNECTED) {
+            Ctx->HandshakeCompleteEvent.Set();
+        } else if (Event->Type == QUIC_CONNECTION_EVENT_PEER_ADDRESS_CHANGED) {
+            MsQuicSettings Settings;
+            Conn->GetSettings(&Settings);
+            Settings.IsSetFlags = 0;
+            Settings.SetPeerBidiStreamCount(Settings.PeerBidiStreamCount + 1);
+            Conn->SetSettings(Settings);
+            Ctx->PeerAddrChangedEvent.Set();
+        }
+        return QUIC_STATUS_SUCCESS;
+    }
+};
+
+static
+QUIC_STATUS
+QUIC_API
+ClientCallback(
+    _In_ MsQuicConnection* /* Connection */,
+    _In_opt_ void* Context,
+    _Inout_ QUIC_CONNECTION_EVENT* Event
+    ) noexcept
+{
+    if (Event->Type == QUIC_CONNECTION_EVENT_PEER_STREAM_STARTED) {
+        MsQuic->StreamClose(Event->PEER_STREAM_STARTED.Stream);
+    } else if (Event->Type == QUIC_CONNECTION_EVENT_STREAMS_AVAILABLE) {
+        CxPlatEvent* StreamCountEvent = static_cast<CxPlatEvent*>(Context);
+        StreamCountEvent->Set();
+    }
+    return QUIC_STATUS_SUCCESS;
+}
+
+void
+QuicTestLocalPathChanges(
+    _In_ int Family
+    )
+{
+    PathTestContext Context;
+    CxPlatEvent PeerStreamsChanged;
+    MsQuicRegistration Registration{true};
+    TEST_QUIC_SUCCEEDED(Registration.GetInitStatus());
+
+    MsQuicSettings Settings;
+    Settings.SetMinimumMtu(1280).SetMaximumMtu(1280);
+
+    MsQuicConfiguration ServerConfiguration(Registration, "MsQuicTest", Settings, ServerSelfSignedCredConfig);
+    TEST_QUIC_SUCCEEDED(ServerConfiguration.GetInitStatus());
+
+    MsQuicConfiguration ClientConfiguration(Registration, "MsQuicTest", Settings, MsQuicCredentialConfig{});
+    TEST_QUIC_SUCCEEDED(ClientConfiguration.GetInitStatus());
+
+    MsQuicAutoAcceptListener Listener(Registration, ServerConfiguration, PathTestContext::ConnCallback, &Context);
+    TEST_QUIC_SUCCEEDED(Listener.GetInitStatus());
+    QUIC_ADDRESS_FAMILY QuicAddrFamily = (Family == 4) ? QUIC_ADDRESS_FAMILY_INET : QUIC_ADDRESS_FAMILY_INET6;
+    QuicAddr ServerLocalAddr(QuicAddrFamily);
+    TEST_QUIC_SUCCEEDED(Listener.Start("MsQuicTest", &ServerLocalAddr.SockAddr));
+    TEST_QUIC_SUCCEEDED(Listener.GetLocalAddr(ServerLocalAddr));
+
+    MsQuicConnection Connection(Registration, CleanUpManual, ClientCallback, &PeerStreamsChanged);
+    TEST_QUIC_SUCCEEDED(Connection.GetInitStatus());
+
+    TEST_QUIC_SUCCEEDED(Connection.StartLocalhost(ClientConfiguration, ServerLocalAddr));
+    TEST_TRUE(Connection.HandshakeCompleteEvent.WaitTimeout(TestWaitTimeout));
+    TEST_NOT_EQUAL(nullptr, Context.Connection);
+    TEST_TRUE(Context.Connection->HandshakeCompleteEvent.WaitTimeout(TestWaitTimeout));
+
+    QuicAddr OrigLocalAddr;
+    TEST_QUIC_SUCCEEDED(Connection.GetLocalAddr(OrigLocalAddr));
+    ReplaceAddressHelper AddrHelper(OrigLocalAddr.SockAddr, OrigLocalAddr.SockAddr);
+
+    for (int i = 0; i < 50; i++) {
+        QuicAddrSetPort(&AddrHelper.New, QuicAddrGetPort(&AddrHelper.New) + 1);
+        Connection.SetSettings(MsQuicSettings{}.SetKeepAlive(25));
+
+        TEST_TRUE(Context.PeerAddrChangedEvent.WaitTimeout(1500));
+        Context.PeerAddrChangedEvent.Reset();
+        QuicAddr ServerRemoteAddr;
+        TEST_QUIC_SUCCEEDED(Context.Connection->GetRemoteAddr(ServerRemoteAddr));
+        TEST_TRUE(QuicAddrCompare(&AddrHelper.New, &ServerRemoteAddr.SockAddr));
+        Connection.SetSettings(MsQuicSettings{}.SetKeepAlive(0));
+        TEST_TRUE(PeerStreamsChanged.WaitTimeout(1500));
+        PeerStreamsChanged.Reset();
+    }
+}

src/test/lib/testlib.kernel.vcxproj

@@ -43,6 +43,7 @@
     <ClCompile Include="EventTest.cpp" />
     <ClCompile Include="HandshakeTest.cpp" />
     <ClCompile Include="MtuTest.cpp" />
+    <ClCompile Include="PathTest.cpp" />
     <ClCompile Include="QuicDrill.cpp" />
     <ClCompile Include="TestConnection.cpp" />
     <ClCompile Include="TestListener.cpp" />