From 394f51f46a3cb4ffd21d99bcca3c1b2e65e32b83 Mon Sep 17 00:00:00 2001 From: Anthony Rossi Date: Sat, 15 Jan 2022 17:27:15 -0800 Subject: [PATCH 1/4] Share binding between client and server --- src/core/binding.c | 19 +++++++++++-------- src/core/binding.h | 11 +++++++---- src/core/library.c | 3 +-- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/core/binding.c b/src/core/binding.c index 02ae9b71d3..87a3971613 100644 --- a/src/core/binding.c +++ b/src/core/binding.c @@ -58,6 +58,7 @@ QuicBindingInitialize( Binding->RefCount = 0; // No refs until it's added to the library's list Binding->Exclusive = !(UdpConfig->Flags & CXPLAT_SOCKET_FLAG_SHARE); Binding->ServerOwned = !!(UdpConfig->Flags & CXPLAT_SOCKET_SERVER_OWNED); + Binding->ClientOwned = !(UdpConfig->Flags & CXPLAT_SOCKET_SERVER_OWNED); Binding->Connected = UdpConfig->RemoteAddress == NULL ? FALSE : TRUE; Binding->StatelessOperCount = 0; CxPlatDispatchRwLockInitialize(&Binding->RwLock); @@ -1422,20 +1423,22 @@ QuicBindingDeliverDatagrams( // packet, then the packet is dropped. // - QUIC_CONNECTION* Connection; - if (!Binding->ServerOwned || Packet->IsShortHeader) { - Connection = - QuicLookupFindConnectionByLocalCid( - &Binding->Lookup, - Packet->DestCid, - Packet->DestCidLen); - } else { + QUIC_CONNECTION* Connection = NULL; + + if (Binding->ServerOwned) { Connection = QuicLookupFindConnectionByRemoteHash( &Binding->Lookup, &DatagramChain->Route->RemoteAddress, Packet->SourceCidLen, Packet->SourceCid); + } + if (Connection == NULL && (Binding->ClientOwned || Packet->IsShortHeader)) { + Connection = + QuicLookupFindConnectionByLocalCid( + &Binding->Lookup, + Packet->DestCid, + Packet->DestCidLen); } if (Connection == NULL) { diff --git a/src/core/binding.h b/src/core/binding.h index 77759af39c..a16a05ec33 100644 --- a/src/core/binding.h +++ b/src/core/binding.h @@ -178,13 +178,16 @@ typedef struct QUIC_BINDING { BOOLEAN Exclusive : 1; // - // Indicates whether the binding is owned by the server side (i.e. listener - // and server connections) or by the client side. Different receive side - // logic is used for each, so the binding cannot be shared between clients - // and servers. + // Indicates whether the binding has server side (i.e. listener + // and server connections) owners. // BOOLEAN ServerOwned : 1; + // + // Indicates whether this binding has client side owners or not. + // + BOOLEAN ClientOwned : 1; + // // Indicates that the binding is also explicitly connected to a remote // address, effectively fixing the 4-tuple of the binding. diff --git a/src/core/library.c b/src/core/library.c index 0429d1aba3..8d36c6696f 100644 --- a/src/core/library.c +++ b/src/core/library.c @@ -1526,8 +1526,7 @@ QuicLibraryGetBinding( UdpConfig->LocalAddress, UdpConfig->RemoteAddress); if (Binding != NULL) { - if (!ShareBinding || Binding->Exclusive || - (ServerOwned != Binding->ServerOwned)) { + if (!ShareBinding || Binding->Exclusive) { // // The binding does already exist, but cannot be shared with the // requested configuration. From 77aaea7b41917304d98bfd41c3cf6b83d6acefbc Mon Sep 17 00:00:00 2001 From: Anthony Rossi Date: Sat, 15 Jan 2022 17:31:33 -0800 Subject: [PATCH 2/4] Add small optimization to not create connections on client --- src/core/binding.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/binding.c b/src/core/binding.c index 87a3971613..c4a9468ef0 100644 --- a/src/core/binding.c +++ b/src/core/binding.c @@ -1441,7 +1441,7 @@ QuicBindingDeliverDatagrams( Packet->DestCidLen); } - if (Connection == NULL) { + if (Connection == NULL && Binding->ServerOwned) { // // Because the packet chain is ordered by control packets first, we From 14320f8b52263f66b36080f3dc4df117dd32f73f Mon Sep 17 00:00:00 2001 From: Anthony Rossi Date: Sat, 15 Jan 2022 17:53:20 -0800 Subject: [PATCH 3/4] Remove unused variable --- src/core/library.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/library.c b/src/core/library.c index 8d36c6696f..fbd83c2596 100644 --- a/src/core/library.c +++ b/src/core/library.c @@ -1486,7 +1486,6 @@ QuicLibraryGetBinding( QUIC_ADDR NewLocalAddress; BOOLEAN PortUnspecified = UdpConfig->LocalAddress == NULL || QuicAddrGetPort(UdpConfig->LocalAddress) == 0; BOOLEAN ShareBinding = !!(UdpConfig->Flags & CXPLAT_SOCKET_FLAG_SHARE); - BOOLEAN ServerOwned = !!(UdpConfig->Flags & CXPLAT_SOCKET_SERVER_OWNED); #ifdef QUIC_SHARED_EPHEMERAL_WORKAROUND // From 9fcb18b66ce4b052698928202eb024609067327a Mon Sep 17 00:00:00 2001 From: Anthony Rossi Date: Tue, 18 Jan 2022 13:58:36 -0800 Subject: [PATCH 4/4] Add test. Still broken; further investigation needed. --- src/core/binding.c | 26 ++++++---- src/core/library.c | 5 +- src/test/MsQuicTests.h | 10 +++- src/test/bin/quic_gtest.cpp | 9 ++++ src/test/bin/winkernel/control.cpp | 7 ++- src/test/lib/HandshakeTest.cpp | 80 ++++++++++++++++++++++++++++++ submodules/openssl | 2 +- 7 files changed, 125 insertions(+), 14 deletions(-) diff --git a/src/core/binding.c b/src/core/binding.c index c4a9468ef0..4672d44d69 100644 --- a/src/core/binding.c +++ b/src/core/binding.c @@ -1423,25 +1423,31 @@ QuicBindingDeliverDatagrams( // packet, then the packet is dropped. // - QUIC_CONNECTION* Connection = NULL; + QUIC_CONNECTION* Connection; - if (Binding->ServerOwned) { + if (Binding->ClientOwned || Packet->IsShortHeader) { + Connection = + QuicLookupFindConnectionByLocalCid( + &Binding->Lookup, + Packet->DestCid, + Packet->DestCidLen); + } else { Connection = QuicLookupFindConnectionByRemoteHash( &Binding->Lookup, &DatagramChain->Route->RemoteAddress, Packet->SourceCidLen, Packet->SourceCid); - } - if (Connection == NULL && (Binding->ClientOwned || Packet->IsShortHeader)) { - Connection = - QuicLookupFindConnectionByLocalCid( - &Binding->Lookup, - Packet->DestCid, - Packet->DestCidLen); + if (Connection == NULL && Binding->ClientOwned) { + Connection = + QuicLookupFindConnectionByLocalCid( + &Binding->Lookup, + Packet->DestCid, + Packet->DestCidLen); + } } - if (Connection == NULL && Binding->ServerOwned) { + if (Connection == NULL) { // // Because the packet chain is ordered by control packets first, we diff --git a/src/core/library.c b/src/core/library.c index fbd83c2596..da7e891ec8 100644 --- a/src/core/library.c +++ b/src/core/library.c @@ -1464,7 +1464,7 @@ QuicLibraryLookupBinding( continue; } - } else if (RemoteAddress != NULL) { + } else if (RemoteAddress != NULL) { continue; } @@ -1541,6 +1541,9 @@ QuicLibraryGetBinding( // Match found and can be shared. // CXPLAT_DBG_ASSERT(Binding->RefCount > 0); + if (!Binding->ClientOwned && !!(UdpConfig->Flags & CXPLAT_SOCKET_SERVER_OWNED)) { + Binding->ClientOwned = TRUE; + } Binding->RefCount++; *NewBinding = Binding; Status = QUIC_STATUS_SUCCESS; diff --git a/src/test/MsQuicTests.h b/src/test/MsQuicTests.h index 59eadfe964..0d79023c86 100644 --- a/src/test/MsQuicTests.h +++ b/src/test/MsQuicTests.h @@ -204,6 +204,11 @@ QuicTestInterfaceBinding( _In_ int Family ); +void +QuicTestClientServerSharedBinding( + _In_ int Family + ); + // // Negative Handshake Tests // @@ -967,4 +972,7 @@ typedef struct { #define IOCTL_QUIC_RUN_REG_SHUTDOWN_AFTER_OPEN_AND_START \ QUIC_CTL_CODE(83, METHOD_BUFFERED, FILE_WRITE_DATA) -#define QUIC_MAX_IOCTL_FUNC_CODE 83 +#define IOCTL_QUIC_RUN_CLIENT_SERVER_SHARED_BINDING \ + QUIC_CTL_CODE(84, METHOD_BUFFERED, FILE_WRITE_DATA) + +#define QUIC_MAX_IOCTL_FUNC_CODE 84 diff --git a/src/test/bin/quic_gtest.cpp b/src/test/bin/quic_gtest.cpp index bfc73865ef..1e04c7f2d4 100644 --- a/src/test/bin/quic_gtest.cpp +++ b/src/test/bin/quic_gtest.cpp @@ -1602,6 +1602,15 @@ TEST_P(WithFamilyArgs, DatagramSend) { } } +TEST_P(WithFamilyArgs, ClientServerSharedBinding) { + TestLoggerT Logger("QuicTestClientServerSharedBinding", GetParam()); + if (TestingKernelMode) { + ASSERT_TRUE(DriverClient.Run(IOCTL_QUIC_RUN_CLIENT_SERVER_SHARED_BINDING, GetParam().Family)); + } else { + QuicTestClientServerSharedBinding(GetParam().Family); + } +} + INSTANTIATE_TEST_SUITE_P( ParameterValidation, WithBool, diff --git a/src/test/bin/winkernel/control.cpp b/src/test/bin/winkernel/control.cpp index 92ffb8a6a4..2f0b068c43 100644 --- a/src/test/bin/winkernel/control.cpp +++ b/src/test/bin/winkernel/control.cpp @@ -453,7 +453,8 @@ size_t QUIC_IOCTL_BUFFER_SIZES[] = 0, 0, 0, - 0 + 0, + sizeof(INT32) }; CXPLAT_STATIC_ASSERT( @@ -1145,6 +1146,10 @@ QuicTestCtlEvtIoDeviceControl( QuicTestCtlRun(QuicTestRegistrationShutdownAfterConnOpenAndStart()); break; + case IOCTL_QUIC_RUN_CLIENT_SERVER_SHARED_BINDING: + QuicTestCtlRun(QuicTestClientServerSharedBinding(Params->Family)); + break; + default: Status = STATUS_NOT_IMPLEMENTED; break; diff --git a/src/test/lib/HandshakeTest.cpp b/src/test/lib/HandshakeTest.cpp index e107c88b07..d8c0da3e20 100644 --- a/src/test/lib/HandshakeTest.cpp +++ b/src/test/lib/HandshakeTest.cpp @@ -2804,3 +2804,83 @@ QuicTestInterfaceBinding( Connection2.HandshakeCompleteEvent.WaitTimeout(TestWaitTimeout); TEST_TRUE(!Connection2.HandshakeComplete); } + +void +QuicTestClientServerSharedBinding( + _In_ int Family + ) +{ + MsQuicRegistration Registration; + TEST_TRUE(Registration.IsValid()); + + MsQuicAlpn Alpn("MsQuicTest"); + + MsQuicSettings Settings; + Settings.SetIdleTimeoutMs(3000); + QUIC_ADDRESS_FAMILY QuicAddrFamily = (Family == 4) ? QUIC_ADDRESS_FAMILY_INET : QUIC_ADDRESS_FAMILY_INET6; + + MsQuicConfiguration ServerConfiguration(Registration, Alpn, Settings, ServerSelfSignedCredConfig); + TEST_TRUE(ServerConfiguration.IsValid()); + + MsQuicCredentialConfig ClientCredConfig; + MsQuicConfiguration ClientConfiguration(Registration, Alpn, Settings, ClientCredConfig); + TEST_TRUE(ClientConfiguration.IsValid()); + + { + TestListener Listener(Registration, ListenerAcceptConnection, ServerConfiguration); + TEST_TRUE(Listener.IsValid()); + QuicAddr ServerLocalAddr(QuicAddrFamily); + TEST_QUIC_SUCCEEDED(Listener.Start(Alpn, &ServerLocalAddr.SockAddr)); + + TEST_QUIC_SUCCEEDED(Listener.GetLocalAddr(ServerLocalAddr)); + TEST_EQUAL(QuicAddrGetFamily(&ServerLocalAddr.SockAddr), QuicAddrFamily); + + { + UniquePtr Server; + ServerAcceptContext ServerAcceptCtx((TestConnection**)&Server); + Listener.Context = &ServerAcceptCtx; + + { + TestConnection Client(Registration); + TEST_TRUE(Client.IsValid()); + + TEST_QUIC_SUCCEEDED(Client.SetShareUdpBinding(true)); + if (QuicAddrFamily == QUIC_ADDRESS_FAMILY_INET) { + CxPlatZeroMemory( + &ServerLocalAddr.SockAddr.Ipv4.sin_addr, + sizeof(ServerLocalAddr.SockAddr.Ipv4.sin_addr)); + } else { + TEST_EQUAL(QuicAddrFamily, QUIC_ADDRESS_FAMILY_INET6); + CxPlatZeroMemory( + &ServerLocalAddr.SockAddr.Ipv6.sin6_addr, + sizeof(ServerLocalAddr.SockAddr.Ipv6.sin6_addr)); + } + TEST_QUIC_SUCCEEDED(Client.SetLocalAddr(ServerLocalAddr)); + + TEST_QUIC_SUCCEEDED( + Client.Start( + ClientConfiguration, + QuicAddrFamily, + QUIC_LOCALHOST_FOR_AF(QuicAddrFamily), + ServerLocalAddr.GetPort())); + + if (!Client.WaitForConnectionComplete()) { + return; + } + TEST_TRUE(Client.GetIsConnected()); + + TEST_NOT_EQUAL(nullptr, Server); + if (!Server->WaitForConnectionComplete()) { + return; + } + TEST_TRUE(Server->GetIsConnected()); + + Client.Shutdown(QUIC_CONNECTION_SHUTDOWN_FLAG_NONE, QUIC_STATUS_SUCCESS); + + TEST_TRUE(Client.GetIsShutdown()); + TEST_TRUE(Server->GetIsShutdown()); + TEST_TRUE(Server->GetPeerClosed()); + } + } + } +} diff --git a/submodules/openssl b/submodules/openssl index 7c0006ccf8..df92821e31 160000 --- a/submodules/openssl +++ b/submodules/openssl @@ -1 +1 @@ -Subproject commit 7c0006ccf891c20cd0b1e9e6a436f9d1f3153b7b +Subproject commit df92821e310497f8664b30cff24da897996d3cce