Apply the unconnected socket requirements to added paths (#63)

Description
QUIC_PARAM_CONN_UNCONNECTED_UDP_SOCKET was only honoured on the connection's first path. QuicConnOpenNewPath, which opens the binding for every path added afterwards with QUIC_PARAM_CONN_ADD_PATH, still passed the path's remote address to QuicLibraryGetBinding:

    if (QuicAddrIsWildCard(&Path->Route.RemoteAddress) && QuicAddrGetPort(&Path->Route.RemoteAddress) == 0) {
        UdpConfig.RemoteAddress = &Connection->Paths[0].Route.RemoteAddress;
    } else {
        UdpConfig.RemoteAddress = &Path->Route.RemoteAddress;
    }
So the added path got a connected socket and a binding of its own, even though the connection had asked for the opposite. A connection using the parameter to keep several destinations on one local port would silently get a second local port as soon as it added a path.

Changes
UdpConfig.RemoteAddress is left NULL when the parameter is set, which is what marks the binding unconnected and lets the lookup match on local port alone.

The two requirements QuicConnStart already enforces are enforced here too, before the binding is built:

A shared binding. An unconnected socket receives datagrams from any remote address, so packets are matched to a connection by connection ID alone. Returns QUIC_STATUS_INVALID_STATE.
A specific local address. An unconnected socket has no source address of its own, and the path's first packet goes out before anything has been learned from the peer. Returns QUIC_STATUS_INVALID_PARAMETER.
Both log through ConnError with the same wording QuicConnStart uses. The early goto Error is safe: the label only releases PathID, which is NULL at that point.

Status code change
The local address requirement now reports QUIC_STATUS_INVALID_PARAMETER rather than QUIC_STATUS_INVALID_STATE, in QuicConnStart as well. It describes an address the caller passed in, not a state the connection is in. docs/Settings.md and the existing test are updated to match.

Testing
Two tests, because ADD_PATH reaches this code by two different routes.

QuicTestUnconnectedSocketAddPathBeforeStart — before the connection is started, QuicConnAddPath configures Paths[0] and returns without opening a binding (if (!Connection->State.Connected) goto Done;), so the address it sets has to satisfy QuicConnStart's requirement instead. Covers a wildcard local address being rejected, and a specific one connecting on the address that was named.

QuicTestUnconnectedSocketAddPathAfterStart — after the handshake, QuicConnAddPath runs QuicConnOpenNewPath and the binding is opened there. Two listeners are used so the added path has a remote address a connected socket could not have reached. Covers:

Requested	Expected	Enforced by
same local + same remote	QUIC_STATUS_ADDRESS_IN_USE	QuicConnAddPath's duplicate check
wildcard local + other remote	QUIC_STATUS_INVALID_PARAMETER	the new check in QuicConnOpenNewPath
same local + other remote	success, local port unchanged	UdpConfig.RemoteAddress = NULL
Both new checks were verified to be the ones failing the tests by disabling each in turn and watching the corresponding case flip to a failure.

Results:

*UnconnectedSocket*:*Path*:*Migration*:*ConnectionParam*:*Datagram* passes in full: 112 tests.
*Basic* passes in full: 497 tests.
No compiler warnings.
Not covered: the !ShareBinding branch in QuicConnOpenNewPath. Setting QUIC_PARAM_CONN_UNCONNECTED_UDP_SOCKET requires a shared binding, and QUIC_PARAM_CONN_SHARE_UDP_BINDING cannot be changed once the connection is started, so the combination is unreachable through the API. It stays as a guard against a binding that was un-shared afterwards.

Documentation
docs/Settings.md records the corrected status code and notes that each additional path opened with QUIC_PARAM_CONN_ADD_PATH needs a specific local address for the same reason.

Author: masa-koz

docs/Settings.md

@@ -229,7 +229,7 @@ By default a client connection's UDP socket is connected to the server's address
 
 The parameter requires `QUIC_PARAM_CONN_SHARE_UDP_BINDING` to also be set: an unconnected socket receives datagrams from any remote address, so incoming packets are matched to a connection by connection ID alone, and only a shared binding gives the connection a non-zero length source connection ID. Setting it without one fails with `QUIC_STATUS_INVALID_STATE`.
 
-It also requires a specific local address, set with `QUIC_PARAM_CONN_LOCAL_ADDRESS`. A connected socket takes its source address from the kernel when it is connected; an unconnected one does not, and the connection's first packet goes out before anything has been learned from the peer, so the address to send from has to be named. The port may be left as 0 to let the stack choose one. Starting a connection with an unconnected socket and no local address, or a wildcard one, fails the connection with `QUIC_STATUS_INVALID_STATE`.
+It also requires a specific local address, set with `QUIC_PARAM_CONN_LOCAL_ADDRESS`. A connected socket takes its source address from the kernel when it is connected; an unconnected one does not, and the connection's first packet goes out before anything has been learned from the peer, so the address to send from has to be named. The port may be left as 0 to let the stack choose one. Starting a connection with an unconnected socket and no local address, or a wildcard one, fails the connection with `QUIC_STATUS_INVALID_PARAMETER`. The same applies to each additional path opened with `QUIC_PARAM_CONN_ADD_PATH`, whose local address must likewise be a specific one.
 
 To place several connections on one local port, start the first connection, read its local address back with `QUIC_PARAM_CONN_LOCAL_ADDRESS`, and set that address on the subsequent connections along with the same two parameters.
 

src/core/connection.c

@@ -1666,7 +1666,7 @@ QuicConnStart(
             // goes out before anything has been learned from the peer, so the
             // application has to name the address to send from.
             //
-            Status = QUIC_STATUS_INVALID_STATE;
+            Status = QUIC_STATUS_INVALID_PARAMETER;
             QuicTraceEvent(
                 ConnError,
                 "[conn][%p] ERROR, %s.",
@@ -6844,12 +6844,51 @@ QuicConnOpenNewPath(
 
     QUIC_BINDING* NewBinding = NULL;
     CXPLAT_UDP_CONFIG UdpConfig = {0};
+
+    if (Connection->State.UnconnectedSocket) {
+        if (!Connection->State.ShareBinding) {
+            //
+            // Setting the parameter requires a shared binding, so this only
+            // catches a binding that was un-shared afterwards.
+            //
+            Status = QUIC_STATUS_INVALID_STATE;
+            QuicTraceEvent(
+                ConnError,
+                "[conn][%p] ERROR, %s.",
+                Connection,
+                "Unconnected socket requires a shared binding");
+            goto Error;
+        }
+
+        if (QuicAddrIsWildCard(&Path->Route.LocalAddress)) {
+            //
+            // A connected socket takes its source address from the kernel when
+            // it is connected. An unconnected one does not, and the path's first
+            // packet goes out before anything has been learned from the peer, so
+            // the caller has to name the address to send from.
+            //
+            Status = QUIC_STATUS_INVALID_PARAMETER;
+            QuicTraceEvent(
+                ConnError,
+                "[conn][%p] ERROR, %s.",
+                Connection,
+                "Unconnected socket requires a specific local address");
+            goto Error;
+        }
+    }
+
     if (QuicAddrIsWildCard(&Path->Route.LocalAddress) && QuicAddrGetPort(&Path->Route.LocalAddress) == 0) {
         UdpConfig.LocalAddress = NULL;
     } else {
         UdpConfig.LocalAddress = &Path->Route.LocalAddress;
     }
-    if (QuicAddrIsWildCard(&Path->Route.RemoteAddress) && QuicAddrGetPort(&Path->Route.RemoteAddress) == 0) {
+    if (Connection->State.UnconnectedSocket) {
+        //
+        // Passing no remote address leaves the socket unconnected, which is what
+        // lets a single binding carry connections to different remote addresses.
+        //
+        UdpConfig.RemoteAddress = NULL;
+    } else if (QuicAddrIsWildCard(&Path->Route.RemoteAddress) && QuicAddrGetPort(&Path->Route.RemoteAddress) == 0) {
         UdpConfig.RemoteAddress = &Connection->Paths[0].Route.RemoteAddress;
     } else {
         UdpConfig.RemoteAddress = &Path->Route.RemoteAddress;

src/test/MsQuicTests.h

@@ -158,6 +158,8 @@ void QuicTestAddrFunctions(const FamilyArgs& Params);
 #ifdef QUIC_API_ENABLE_PREVIEW_FEATURES
 void QuicTestConnectUnconnectedSocket(const FamilyArgs& Params);
 void QuicTestUnconnectedSocketRequirements();
+void QuicTestUnconnectedSocketAddPathBeforeStart(const FamilyArgs& Params);
+void QuicTestUnconnectedSocketAddPathAfterStart(const FamilyArgs& Params);
 #endif
 
 //

src/test/bin/quic_gtest.cpp

@@ -1062,6 +1062,24 @@ TEST_P(WithFamilyArgs, ConnectUnconnectedSocket) {
     }
 }
 
+TEST_P(WithFamilyArgs, UnconnectedSocketAddPathBeforeStart) {
+    TestLoggerT<ParamType> Logger("QuicTestUnconnectedSocketAddPathBeforeStart", GetParam());
+    if (TestingKernelMode) {
+        ASSERT_TRUE(InvokeKernelTest(FUNC(QuicTestUnconnectedSocketAddPathBeforeStart), GetParam()));
+    } else {
+        QuicTestUnconnectedSocketAddPathBeforeStart(GetParam());
+    }
+}
+
+TEST_P(WithFamilyArgs, UnconnectedSocketAddPathAfterStart) {
+    TestLoggerT<ParamType> Logger("QuicTestUnconnectedSocketAddPathAfterStart", GetParam());
+    if (TestingKernelMode) {
+        ASSERT_TRUE(InvokeKernelTest(FUNC(QuicTestUnconnectedSocketAddPathAfterStart), GetParam()));
+    } else {
+        QuicTestUnconnectedSocketAddPathAfterStart(GetParam());
+    }
+}
+
 TEST(Basic, UnconnectedSocketRequirements) {
     TestLogger Logger("QuicTestUnconnectedSocketRequirements");
     if (TestingKernelMode) {

src/test/bin/winkernel/control.cpp

@@ -522,6 +522,8 @@ ExecuteTestRequest(
 #ifdef QUIC_API_ENABLE_PREVIEW_FEATURES
     RegisterTestFunction(QuicTestConnectUnconnectedSocket);
     RegisterTestFunction(QuicTestUnconnectedSocketRequirements);
+    RegisterTestFunction(QuicTestUnconnectedSocketAddPathBeforeStart);
+    RegisterTestFunction(QuicTestUnconnectedSocketAddPathAfterStart);
 #endif
     RegisterTestFunction(QuicTestConnect_Connect);
 #ifndef QUIC_DISABLE_RESUMPTION

src/test/lib/BasicTest.cpp

@@ -492,7 +492,197 @@ void QuicTestUnconnectedSocketRequirements()
 
         TEST_TRUE(Connection.HandshakeCompleteEvent.WaitTimeout(TestWaitTimeout));
         TEST_FALSE(Connection.HandshakeComplete);
-        TEST_EQUAL(QUIC_STATUS_INVALID_STATE, Connection.TransportShutdownStatus);
+        TEST_EQUAL(QUIC_STATUS_INVALID_PARAMETER, Connection.TransportShutdownStatus);
+    }
+}
+
+//
+// Sets the connection's local and remote address with QUIC_PARAM_CONN_ADD_PATH
+// instead of QUIC_PARAM_CONN_LOCAL_ADDRESS/REMOTE_ADDRESS. Before the
+// connection is started, ADD_PATH configures Paths[0] rather than opening an
+// additional path, so it has to satisfy the unconnected socket's requirement
+// for a specific local address the same way the individual parameters do.
+//
+void QuicTestUnconnectedSocketAddPathBeforeStart(const FamilyArgs& Params)
+{
+    const int Family = Params.Family;
+    const QUIC_ADDRESS_FAMILY QuicAddrFamily =
+        (Family == 4) ? QUIC_ADDRESS_FAMILY_INET : QUIC_ADDRESS_FAMILY_INET6;
+
+    MsQuicRegistration Registration(true);
+    TEST_QUIC_SUCCEEDED(Registration.GetInitStatus());
+
+    MsQuicConfiguration ServerConfiguration(Registration, "MsQuicTest", ServerSelfSignedCredConfig);
+    TEST_QUIC_SUCCEEDED(ServerConfiguration.GetInitStatus());
+
+    MsQuicConfiguration ClientConfiguration(Registration, "MsQuicTest", MsQuicCredentialConfig());
+    TEST_QUIC_SUCCEEDED(ClientConfiguration.GetInitStatus());
+
+    MsQuicAutoAcceptListener Listener(Registration, ServerConfiguration, MsQuicConnection::NoOpCallback);
+    TEST_QUIC_SUCCEEDED(Listener.GetInitStatus());
+    TEST_QUIC_SUCCEEDED(Listener.Start("MsQuicTest"));
+    QuicAddr ServerAddr;
+    TEST_QUIC_SUCCEEDED(Listener.GetLocalAddr(ServerAddr));
+
+    QuicAddr LocalAddr(QuicAddrFamily, true);
+    QuicAddr RemoteAddr(QuicAddrFamily, true);
+    if (UseDuoNic) {
+        QuicAddrSetToDuoNic(&LocalAddr.SockAddr);
+        QuicAddrSetToDuoNic(&RemoteAddr.SockAddr);
+    }
+    RemoteAddr.SetPort(ServerAddr.GetPort());
+
+    QUIC_PATH_PARAM PathParam = { &LocalAddr.SockAddr, &RemoteAddr.SockAddr };
+
+    //
+    // A wildcard local address is rejected, since an unconnected socket has no
+    // source address of its own to send the connection's first packet from.
+    //
+    {
+        QuicAddr WildcardLocalAddr(QuicAddrFamily);
+        QUIC_PATH_PARAM WildcardParam = { &WildcardLocalAddr.SockAddr, &RemoteAddr.SockAddr };
+
+        MsQuicConnection Connection(Registration);
+        TEST_QUIC_SUCCEEDED(Connection.GetInitStatus());
+        TEST_QUIC_SUCCEEDED(Connection.SetShareUdpBinding());
+        TEST_QUIC_SUCCEEDED(Connection.SetUnconnectedUdpSocket());
+        TEST_QUIC_SUCCEEDED(
+            Connection.SetParam(QUIC_PARAM_CONN_ADD_PATH, sizeof(WildcardParam), &WildcardParam));
+        TEST_QUIC_SUCCEEDED(
+            Connection.Start(ClientConfiguration, QuicAddrFamily, nullptr, ServerAddr.GetPort()));
+
+        TEST_TRUE(Connection.HandshakeCompleteEvent.WaitTimeout(TestWaitTimeout));
+        TEST_FALSE(Connection.HandshakeComplete);
+        TEST_EQUAL(QUIC_STATUS_INVALID_PARAMETER, Connection.TransportShutdownStatus);
+    }
+
+    //
+    // With a specific one the connection is established, on the address that
+    // was named.
+    //
+    {
+        MsQuicConnection Connection(Registration);
+        TEST_QUIC_SUCCEEDED(Connection.GetInitStatus());
+        TEST_QUIC_SUCCEEDED(Connection.SetShareUdpBinding());
+        TEST_QUIC_SUCCEEDED(Connection.SetUnconnectedUdpSocket());
+        TEST_QUIC_SUCCEEDED(
+            Connection.SetParam(QUIC_PARAM_CONN_ADD_PATH, sizeof(PathParam), &PathParam));
+        TEST_QUIC_SUCCEEDED(
+            Connection.Start(ClientConfiguration, QuicAddrFamily, nullptr, ServerAddr.GetPort()));
+
+        TEST_TRUE(Connection.HandshakeCompleteEvent.WaitTimeout(TestWaitTimeout));
+        TEST_TRUE(Connection.HandshakeComplete);
+
+        QuicAddr ActualLocalAddr;
+        TEST_QUIC_SUCCEEDED(Connection.GetLocalAddr(ActualLocalAddr));
+        TEST_FALSE(QuicAddrIsWildCard(&ActualLocalAddr.SockAddr));
+        TEST_NOT_EQUAL(0, ActualLocalAddr.GetPort());
+
+        QuicAddr ActualRemoteAddr;
+        TEST_QUIC_SUCCEEDED(Connection.GetRemoteAddr(ActualRemoteAddr));
+        TEST_EQUAL(ServerAddr.GetPort(), ActualRemoteAddr.GetPort());
+    }
+}
+
+//
+// Adds a path with QUIC_PARAM_CONN_ADD_PATH once the connection is established.
+// Unlike the pre-start case, this opens a binding of its own, so it is the path
+// that exercises the unconnected socket handling in QuicConnOpenNewPath.
+//
+void QuicTestUnconnectedSocketAddPathAfterStart(const FamilyArgs& Params)
+{
+    const int Family = Params.Family;
+    const QUIC_ADDRESS_FAMILY QuicAddrFamily =
+        (Family == 4) ? QUIC_ADDRESS_FAMILY_INET : QUIC_ADDRESS_FAMILY_INET6;
+
+    MsQuicRegistration Registration(true);
+    TEST_QUIC_SUCCEEDED(Registration.GetInitStatus());
+
+    MsQuicConfiguration ServerConfiguration(Registration, "MsQuicTest", ServerSelfSignedCredConfig);
+    TEST_QUIC_SUCCEEDED(ServerConfiguration.GetInitStatus());
+
+    MsQuicConfiguration ClientConfiguration(Registration, "MsQuicTest", MsQuicCredentialConfig());
+    TEST_QUIC_SUCCEEDED(ClientConfiguration.GetInitStatus());
+
+    //
+    // A second server, so the added path has a remote address the connected
+    // socket of the first path could not have reached.
+    //
+    MsQuicAutoAcceptListener Listener1(Registration, ServerConfiguration, MsQuicConnection::NoOpCallback);
+    TEST_QUIC_SUCCEEDED(Listener1.GetInitStatus());
+    TEST_QUIC_SUCCEEDED(Listener1.Start("MsQuicTest"));
+    QuicAddr Server1Addr;
+    TEST_QUIC_SUCCEEDED(Listener1.GetLocalAddr(Server1Addr));
+
+    MsQuicAutoAcceptListener Listener2(Registration, ServerConfiguration, MsQuicConnection::NoOpCallback);
+    TEST_QUIC_SUCCEEDED(Listener2.GetInitStatus());
+    TEST_QUIC_SUCCEEDED(Listener2.Start("MsQuicTest"));
+    QuicAddr Server2Addr;
+    TEST_QUIC_SUCCEEDED(Listener2.GetLocalAddr(Server2Addr));
+
+    TEST_NOT_EQUAL(Server1Addr.GetPort(), Server2Addr.GetPort());
+
+    QuicAddr LocalAddr(QuicAddrFamily, true);
+    if (UseDuoNic) {
+        QuicAddrSetToDuoNic(&LocalAddr.SockAddr);
+    }
+
+    MsQuicConnection Connection(Registration);
+    TEST_QUIC_SUCCEEDED(Connection.GetInitStatus());
+    TEST_QUIC_SUCCEEDED(Connection.SetShareUdpBinding());
+    TEST_QUIC_SUCCEEDED(Connection.SetUnconnectedUdpSocket());
+    TEST_QUIC_SUCCEEDED(Connection.SetLocalAddr(LocalAddr));
+    TEST_QUIC_SUCCEEDED(
+        Connection.Start(
+            ClientConfiguration,
+            QuicAddrFamily,
+            QUIC_TEST_LOOPBACK_FOR_AF(QuicAddrFamily),
+            Server1Addr.GetPort()));
+    TEST_TRUE(Connection.HandshakeCompleteEvent.WaitTimeout(TestWaitTimeout));
+    TEST_TRUE(Connection.HandshakeComplete);
+
+    QuicAddr ClientAddr;
+    TEST_QUIC_SUCCEEDED(Connection.GetLocalAddr(ClientAddr));
+    QuicAddr Peer1Addr;
+    TEST_QUIC_SUCCEEDED(Connection.GetRemoteAddr(Peer1Addr));
+
+    QuicAddr Peer2Addr = Peer1Addr;
+    Peer2Addr.SetPort(Server2Addr.GetPort());
+
+    //
+    // The address pair the connection already runs on is not a new path.
+    //
+    {
+        QUIC_PATH_PARAM PathParam = { &ClientAddr.SockAddr, &Peer1Addr.SockAddr };
+        TEST_QUIC_STATUS(
+            QUIC_STATUS_ADDRESS_IN_USE,
+            Connection.SetParam(QUIC_PARAM_CONN_ADD_PATH, sizeof(PathParam), &PathParam));
+    }
+
+    //
+    // A wildcard local address is rejected: the added path's socket is left
+    // unconnected, so it has no source address of its own to send from.
+    //
+    {
+        QuicAddr WildcardLocalAddr(QuicAddrFamily);
+        QUIC_PATH_PARAM PathParam = { &WildcardLocalAddr.SockAddr, &Peer2Addr.SockAddr };
+        TEST_QUIC_STATUS(
+            QUIC_STATUS_INVALID_PARAMETER,
+            Connection.SetParam(QUIC_PARAM_CONN_ADD_PATH, sizeof(PathParam), &PathParam));
+    }
+
+    //
+    // The same local address towards the second server is a new path, and it
+    // shares the binding the connection is already using.
+    //
+    {
+        QUIC_PATH_PARAM PathParam = { &ClientAddr.SockAddr, &Peer2Addr.SockAddr };
+        TEST_QUIC_SUCCEEDED(
+            Connection.SetParam(QUIC_PARAM_CONN_ADD_PATH, sizeof(PathParam), &PathParam));
+
+        QuicAddr StillClientAddr;
+        TEST_QUIC_SUCCEEDED(Connection.GetLocalAddr(StillClientAddr));
+        TEST_EQUAL(ClientAddr.GetPort(), StillClientAddr.GetPort());
     }
 }