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: