From 5206700067d4a04a4224c4cc2fac1f3e1fdea4a9 Mon Sep 17 00:00:00 2001 From: jyelon Date: Mon, 6 Apr 2026 01:44:21 -0400 Subject: [PATCH] Harden UE Wingman request handling and numeric property conversion. Switch the Wingman protocol to null-delimited JSON, rework the server's socket buffering and send logic, and document the bugs found during the review. Also refactor WingProperty's numeric setters into clearer helper paths while preserving the existing conversion rules. --- .gitignore | 1 + Docs/bugs-in-ue-wingman.md | 13 ++ .../Source/UEWingman/Private/WingProperty.cpp | 148 +++++++++------ .../Source/UEWingman/Private/WingServer.cpp | 179 +++++++++++++----- .../Source/UEWingman/Public/WingProperty.h | 19 +- .../Source/UEWingman/Public/WingServer.h | 10 +- Plugins/UEWingman/ue-wingman-mcp.py | 2 +- Plugins/UEWingman/ue-wingman.py | 2 +- 8 files changed, 264 insertions(+), 110 deletions(-) create mode 100644 Docs/bugs-in-ue-wingman.md diff --git a/.gitignore b/.gitignore index fd4eb291..4771453e 100644 --- a/.gitignore +++ b/.gitignore @@ -51,3 +51,4 @@ GPF-output/** __pycache__/ .clangd-query/ +COMMIT.txt diff --git a/Docs/bugs-in-ue-wingman.md b/Docs/bugs-in-ue-wingman.md new file mode 100644 index 00000000..bf91a60b --- /dev/null +++ b/Docs/bugs-in-ue-wingman.md @@ -0,0 +1,13 @@ +# Bugs in UE Wingman + +- Partial TCP writes are not handled in [`Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp`](/home/jyelon/integration/Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp#L441). The server calls `Socket->Send(...)` once and ignores short writes, so large responses can be truncated and the client can block forever waiting for the terminating null byte. + +- Unsigned numeric properties are validated incorrectly in [`Plugins/UEWingman/Source/UEWingman/Private/WingProperty.cpp`](/home/jyelon/integration/Plugins/UEWingman/Source/UEWingman/Private/WingProperty.cpp#L85) and [`Plugins/UEWingman/Source/UEWingman/Private/WingProperty.cpp`](/home/jyelon/integration/Plugins/UEWingman/Source/UEWingman/Private/WingProperty.cpp#L125). The code always reads back through `GetSignedIntPropertyValue`, which is the wrong check for unsigned properties and can reject valid values. + +- UTF-8 decoding is done per recv chunk instead of on complete messages in [`Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp`](/home/jyelon/integration/Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp#L405). If a multibyte code point is split across TCP packets, the request can be corrupted before JSON parsing. + +- Request execution is serialized to one queued message per editor tick in [`Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp`](/home/jyelon/integration/Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp#L227). Any slow handler blocks every connected client behind it because each client thread waits synchronously for its response. + +- Shutdown can hang if a handler is already in flight in [`Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp`](/home/jyelon/integration/Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp#L188). Pending queued work is drained, but an active game-thread handler has no cancellation path, and client threads still wait on its unresolved promise. + +- The shared output buffer is fixed at 65536 characters in [`Plugins/UEWingman/Source/UEWingman/Public/WingHandler.h`](/home/jyelon/integration/Plugins/UEWingman/Source/UEWingman/Public/WingHandler.h#L75) and instantiated in [`Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp`](/home/jyelon/integration/Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp#L502). Large dumps and manual output can be truncated with no warning. diff --git a/Plugins/UEWingman/Source/UEWingman/Private/WingProperty.cpp b/Plugins/UEWingman/Source/UEWingman/Private/WingProperty.cpp index a392be8d..bb9e1264 100644 --- a/Plugins/UEWingman/Source/UEWingman/Private/WingProperty.cpp +++ b/Plugins/UEWingman/Source/UEWingman/Private/WingProperty.cpp @@ -46,52 +46,61 @@ bool FWingProperty::SetObject(UObject *Obj, WingOut Errors) const return true; } +bool FWingProperty::SetDoubleInternal( + FNumericProperty *NProp, void *Container, double D, WingOut Errors) +{ + uint8 buffer[16]; + NProp->SetFloatingPointPropertyValue(buffer, D); + if (!FMath::IsFinite(NProp->GetFloatingPointPropertyValue(buffer))) + { + Errors.Printf(TEXT("ERROR: Property '%s' of type %s cannot hold %lf\n"), + *WingUtils::FormatName(NProp), *NProp->GetCPPType(), D); + return false; + } + NProp->SetValue_InContainer(Container, buffer); + return true; +} + +bool FWingProperty::SetInt64Internal( + FNumericProperty *NProp, void *Container, int64 I, WingOut Errors) +{ + uint8 buffer[16]; + if ((I < 0) && IsUnsigned(NProp)) + { + Errors.Printf(TEXT( + "ERROR: Cannot store signed %lld in unsigned property %s\n"), + I, *WingUtils::FormatName(NProp)); + return false; + } + NProp->SetIntPropertyValue(buffer, I); + if (NProp->GetSignedIntPropertyValue(buffer) != I) + { + Errors.Printf(TEXT("ERROR: Property '%s' of type %s cannot hold %lld\n"), + *WingUtils::FormatName(NProp), *NProp->GetCPPType(), I); + return false; + } + NProp->SetValue_InContainer(Container, buffer); + return true; +} + bool FWingProperty::SetDouble(double D, WingOut Errors) const { if (!CheckEditable(Errors)) return false; FNumericProperty *NProp = CastField(Prop); - if (!NProp) + if (NProp == nullptr) { PrintExpectsReceived(TEXT("double"), Errors); return false; } - if (NProp->IsFloatingPoint()) + else if (NProp->IsFloatingPoint()) { - uint8 buffer[16]; - NProp->SetFloatingPointPropertyValue(buffer, D); - if (!FMath::IsFinite(NProp->GetFloatingPointPropertyValue(buffer))) - { - Errors.Printf(TEXT("ERROR: Property '%s' of type %s cannot hold %lf\n"), - *WingUtils::FormatName(Prop), *Prop->GetCPPType(), D); - return false; - } - Prop->SetValue_InContainer(Container, buffer); - return true; + return SetDoubleInternal(NProp, Container, D, Errors); } else { - uint8 buffer[16]; - if (FMath::Floor(D) != D) - { - PrintExpectsReceived(TEXT("double"), Errors); - return false; - } - if (FMath::Abs(D) > (double)((1LL)<<53)) - { - Errors.Printf(TEXT("ERROR: To store very large numbers in '%s', do not pass them as double. Use string or int.\n"), - *WingUtils::FormatName(Prop)); - return false; - } - int64 I = (int64)D; - NProp->SetIntPropertyValue(buffer, I); - if (NProp->GetSignedIntPropertyValue(buffer) != I) - { - Errors.Printf(TEXT("ERROR: Property '%s' of type %s cannot hold %lld\n"), - *WingUtils::FormatName(Prop), *Prop->GetCPPType(), I); - return false; - } - NProp->SetValue_InContainer(Container, buffer); - return true; + int64 I; + if (!LosslessDoubleToInt64(D, I, Errors)) return false; + return SetInt64Internal(NProp, Container, I, Errors); } } @@ -99,38 +108,20 @@ bool FWingProperty::SetInt64(int64 I, WingOut Errors) const { if (!CheckEditable(Errors)) return false; FNumericProperty *NProp = CastField(Prop); - if (!NProp) + if (NProp == nullptr) { PrintExpectsReceived(TEXT("int"), Errors); return false; } - if (NProp->IsFloatingPoint()) + else if (NProp->IsFloatingPoint()) { - uint8 buffer[16]; - double D = I; - NProp->SetFloatingPointPropertyValue(buffer, D); - int64 RT = (int64)NProp->GetFloatingPointPropertyValue(buffer); - if (RT != I) - { - Errors.Printf(TEXT("ERROR: Property '%s' of type %s cannot hold %lld\n"), - *WingUtils::FormatName(Prop), *Prop->GetCPPType(), I); - return false; - } - Prop->SetValue_InContainer(Container, buffer); - return true; + double D; + if (!LosslessInt64ToDouble(I, D, Errors)) return false; + return SetDoubleInternal(NProp, Container, D, Errors); } else { - uint8 buffer[16]; - NProp->SetIntPropertyValue(buffer, I); - if (NProp->GetSignedIntPropertyValue(buffer) != I) - { - Errors.Printf(TEXT("ERROR: Property '%s' of type %s cannot hold %lld\n"), - *WingUtils::FormatName(Prop), *Prop->GetCPPType(), I); - return false; - } - NProp->SetValue_InContainer(Container, buffer); - return true; + return SetInt64Internal(NProp, Container, I, Errors); } } @@ -597,4 +588,43 @@ bool FWingProperty::CheckEditable(WingOut Errors) const return false; } return true; -} \ No newline at end of file +} + +bool FWingProperty::IsUnsigned(FNumericProperty* Prop) +{ + return + CastField(Prop) || + CastField(Prop) || + CastField(Prop) || + CastField(Prop); +} + +bool FWingProperty::LosslessDoubleToInt64(double D, int64 &I, WingOut Errors) +{ + if (FMath::Floor(D) != D) + { + Errors.Printf(TEXT( + "ERROR: Converting double %.4lf to integer would lose precision\n"), D); + return false; + } + if (FMath::Abs(D) > (double)((1LL)<<53)) + { + Errors.Printf(TEXT( + "ERROR: Converting huge double %lf to integer would lose data.\n"), D); + return false; + } + I = (int64)D; + return true; +} + +bool FWingProperty::LosslessInt64ToDouble(int64 I, double &D, WingOut Errors) +{ + D = (double)I; + if ((int64)D != I) + { + Errors.Printf(TEXT( + "ERROR: Converting huge integer %lld to floating point would lose data.\n"), I); + return false; + } + return true; +} diff --git a/Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp b/Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp index 09e61bbf..f407f00f 100644 --- a/Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp +++ b/Plugins/UEWingman/Source/UEWingman/Private/WingServer.cpp @@ -384,70 +384,157 @@ void UWingServer::CleanupFinishedClients() } } +// ============================================================ +// Stuff Performed on the Client Thread +// ============================================================ + void UWingServer::ClientThreadFunc(UWingServer* Server, TSharedPtr Client) { + constexpr int32 MaxRecvBufBytes = 1024 * 1024; + constexpr int32 MinUnusedRecvSpace = 4096; + FSocket* Socket = Client->Socket; - FString LineBuffer; - uint8 RecvBuf[4096]; + TArray RecvBuf; + RecvBuf.SetNumUninitialized(MinUnusedRecvSpace); + int32 RecvLen = 0; + + WaitForAssetRegistry(); while (true) { - int32 BytesRead = 0; - if (!Socket->Recv(RecvBuf, sizeof(RecvBuf) - 1, BytesRead)) + FString Request; + if (ExtractRequestFromBuffer(RecvBuf, RecvLen, Request)) { - break; // socket error or closed - } - if (BytesRead <= 0) - { - break; // connection closed - } - - RecvBuf[BytesRead] = 0; - LineBuffer += UTF8_TO_TCHAR((const ANSICHAR*)RecvBuf); - - // Process complete lines - int32 NewlineIdx; - while (LineBuffer.FindChar(TEXT('\n'), NewlineIdx)) - { - FString Line = LineBuffer.Left(NewlineIdx).TrimEnd(); - LineBuffer.RightChopInline(NewlineIdx + 1); - - if (Line.IsEmpty()) continue; - - // Wait for the asset registry to finish its initial scan. + FString Response; + if (!ProcessRequestOnGameThread(Request, Response)) { - IAssetRegistry& AR = FModuleManager::LoadModuleChecked("AssetRegistry").Get(); - while (AR.IsLoadingAssets()) FPlatformProcess::Sleep(0.25f); + Client->bDone = true; + return; } - // Enqueue the line for game-thread processing - TSharedPtr Msg = MakeShared(); - Msg->Line = Line; - TFuture Future = Msg->Response.GetFuture(); - - { - FScopeLock Lock(&Server->Mutex); - if (Server->bShuttingDown) - { - Client->bDone = true; - return; - } - Server->PendingMessages.Add(Msg); - } - - // Block until the game thread processes this message - FString Response = Future.Get(); - // Write the response back, null-terminated (blocking) FTCHARToUTF8 Utf8(*Response); - int32 BytesSent = 0; - Socket->Send((const uint8*)Utf8.Get(), Utf8.Length() + 1, BytesSent); + if (!SendAll(Socket, reinterpret_cast(Utf8.Get()), + Utf8.Length() + 1)) + { + Client->bDone = true; + return; + } + continue; + } + + if (!ReceiveMoreBytesIntoBuffer(Socket, RecvBuf, RecvLen)) + { + break; } } Client->bDone = true; } +bool UWingServer::ExtractRequestFromBuffer( + TArray& RecvBuf, int32& RecvLen, FString& OutRequest) +{ + const uint8* EndOfRequest = static_cast( + memchr(RecvBuf.GetData(), '\0', RecvLen)); + if (EndOfRequest == nullptr) + { + return false; + } + + const int32 MessageLen = + static_cast(EndOfRequest - RecvBuf.GetData()); + OutRequest = FString::ConstructFromPtrSize( + reinterpret_cast(RecvBuf.GetData()), MessageLen); + const int32 RemainingBytes = RecvLen - (MessageLen + 1); + if (RemainingBytes > 0) + { + FMemory::Memmove( + RecvBuf.GetData(), + RecvBuf.GetData() + MessageLen + 1, + RemainingBytes); + } + RecvLen = RemainingBytes; + return true; +} + +bool UWingServer::ReceiveMoreBytesIntoBuffer( + FSocket* Socket, TArray& RecvBuf, int32& RecvLen) +{ + constexpr int32 MaxRecvBufBytes = 1024 * 1024; + constexpr int32 MinUnusedRecvSpace = 4096; + + int32 UnusedSpace = RecvBuf.Num() - RecvLen; + if (UnusedSpace < MinUnusedRecvSpace) + { + if (RecvBuf.Num() >= MaxRecvBufBytes) + { + return false; + } + RecvBuf.SetNumUninitialized(RecvBuf.Num() * 2); + UnusedSpace = RecvBuf.Num() - RecvLen; + } + + int32 BytesRead = 0; + if (!Socket->Recv(RecvBuf.GetData() + RecvLen, UnusedSpace, BytesRead)) + { + return false; + } + if (BytesRead <= 0) + { + return false; + } + + RecvLen += BytesRead; + return true; +} + +bool UWingServer::SendAll(FSocket* Socket, const uint8* Data, int32 BytesToSend) +{ + while (BytesToSend > 0) + { + int32 BytesSent = 0; + if (!Socket->Send(Data, BytesToSend, BytesSent) || (BytesSent <= 0)) + { + return false; + } + Data += BytesSent; + BytesToSend -= BytesSent; + } + return true; +} + +bool UWingServer::ProcessRequestOnGameThread( + const FString& Request, FString& Response) +{ + // Enqueue the message for game-thread processing. + TSharedPtr Msg = + MakeShared(); + Msg->Line = Request; + TFuture Future = Msg->Response.GetFuture(); + + { + FScopeLock Lock(&GWingServer->Mutex); + if (GWingServer->bShuttingDown) + { + return false; + } + GWingServer->PendingMessages.Add(Msg); + } + + // Block until the game thread processes this message. + Response = Future.Get(); + return true; +} + +void UWingServer::WaitForAssetRegistry() +{ + IAssetRegistry& AR = + FModuleManager::LoadModuleChecked( + "AssetRegistry").Get(); + while (AR.IsLoadingAssets()) FPlatformProcess::Sleep(0.25f); +} + // ============================================================ // BuildWingHandlerRegistry // ============================================================ diff --git a/Plugins/UEWingman/Source/UEWingman/Public/WingProperty.h b/Plugins/UEWingman/Source/UEWingman/Public/WingProperty.h index 7096969a..7fcdcd52 100644 --- a/Plugins/UEWingman/Source/UEWingman/Public/WingProperty.h +++ b/Plugins/UEWingman/Source/UEWingman/Public/WingProperty.h @@ -31,8 +31,7 @@ struct FWingProperty // error. These will do automatic conversion of numeric // types to other numeric types, as long as the value // fits. They will also do text to any type, as long as - // the value parses as a value of the desired type. If - // you + // the value parses as a value of the desired type. // bool SetObject(UObject *Obj, WingOut Errors) const; bool SetDouble(double D, WingOut Errors) const; @@ -117,10 +116,26 @@ struct FWingProperty static bool PopulateFromJson(TArray& Props, const FJsonValue& Json, bool AllOptional, WingOut Errors); + // Functions to populate properties from a JSON object. + // + private: static void StripEditable(TArray &Props); + static bool IsUnsigned(FNumericProperty* Prop); static bool IsPinTypeProperty(FProperty *Prop); void PrintExpectsReceived(const TCHAR *Type, WingOut Errors) const; bool CheckImportTextResult(const FString &Value, WingOut Errors) const; bool CheckEditable(WingOut Errors) const; + + // Convert int64 to double losslessly. If it can't be done, generate an error. + static bool LosslessInt64ToDouble(int64 I, double &D, WingOut Errors); + + // Convert double to int64 losslessly. If it can't be done, generate an error. + static bool LosslessDoubleToInt64(double D, int64 &I, WingOut Errors); + + // Unlike SetDouble, this function requires that NProp is a floating point property. + static bool SetDoubleInternal(FNumericProperty *NProp, void *Container, double D, WingOut Errors); + + // Unlike SetInt64, this function requires that NProp is an integral property. + static bool SetInt64Internal(FNumericProperty *NProp, void *Container, int64 I, WingOut Errors); }; diff --git a/Plugins/UEWingman/Source/UEWingman/Public/WingServer.h b/Plugins/UEWingman/Source/UEWingman/Public/WingServer.h index fc1dd44e..76f3d1a7 100644 --- a/Plugins/UEWingman/Source/UEWingman/Public/WingServer.h +++ b/Plugins/UEWingman/Source/UEWingman/Public/WingServer.h @@ -19,7 +19,7 @@ class FSocket; * UWingServer — editor subsystem that listens on a TCP socket and dispatches * JSON commands to blueprint editing handlers. * - * Clients connect via TCP and exchange newline-delimited JSON messages. + * Clients connect via TCP and exchange null-delimited JSON messages. * Request format: {"command": "tool_name", "param1": "value1", ...} * Response format: raw JSON result from the handler. * @@ -98,6 +98,14 @@ private: void AcceptNewConnections(); void CleanupFinishedClients(); static void ClientThreadFunc(UWingServer* Server, TSharedPtr Client); + static bool ExtractRequestFromBuffer( + TArray& RecvBuf, int32& RecvLen, FString& OutRequest); + static bool ReceiveMoreBytesIntoBuffer( + FSocket* Socket, TArray& RecvBuf, int32& RecvLen); + static bool SendAll(FSocket* Socket, const uint8* Data, int32 BytesToSend); + static bool ProcessRequestOnGameThread( + const FString& Request, FString& Response); + static void WaitForAssetRegistry(); // ----- Thread-safe message queue ----- struct FPendingMessage diff --git a/Plugins/UEWingman/ue-wingman-mcp.py b/Plugins/UEWingman/ue-wingman-mcp.py index 8dc4064b..79a0dd21 100644 --- a/Plugins/UEWingman/ue-wingman-mcp.py +++ b/Plugins/UEWingman/ue-wingman-mcp.py @@ -68,7 +68,7 @@ def disconnect(): def send_and_receive(message): """Send a JSON message to the editor and return the null-terminated response.""" - data = json.dumps(message) + "\n" + data = json.dumps(message) + "\0" sock.sendall(data.encode()) result = b"" diff --git a/Plugins/UEWingman/ue-wingman.py b/Plugins/UEWingman/ue-wingman.py index 8ec92525..6a3edd05 100755 --- a/Plugins/UEWingman/ue-wingman.py +++ b/Plugins/UEWingman/ue-wingman.py @@ -41,7 +41,7 @@ def main(): print(f"Cannot connect to {HOST}:{PORT} — is the editor running?") sys.exit(1) - sock.sendall((json.dumps(msg) + "\n").encode()) + sock.sendall((json.dumps(msg) + "\0").encode()) result = b"" while True: