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.
This commit is contained in:
1
.gitignore
vendored
1
.gitignore
vendored
@@ -51,3 +51,4 @@ GPF-output/**
|
||||
|
||||
__pycache__/
|
||||
.clangd-query/
|
||||
COMMIT.txt
|
||||
|
||||
13
Docs/bugs-in-ue-wingman.md
Normal file
13
Docs/bugs-in-ue-wingman.md
Normal file
@@ -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.
|
||||
@@ -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<FNumericProperty>(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<FNumericProperty>(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;
|
||||
}
|
||||
}
|
||||
|
||||
bool FWingProperty::IsUnsigned(FNumericProperty* Prop)
|
||||
{
|
||||
return
|
||||
CastField<FByteProperty>(Prop) ||
|
||||
CastField<FUInt16Property>(Prop) ||
|
||||
CastField<FUInt32Property>(Prop) ||
|
||||
CastField<FUInt64Property>(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;
|
||||
}
|
||||
|
||||
@@ -384,70 +384,157 @@ void UWingServer::CleanupFinishedClients()
|
||||
}
|
||||
}
|
||||
|
||||
// ============================================================
|
||||
// Stuff Performed on the Client Thread
|
||||
// ============================================================
|
||||
|
||||
void UWingServer::ClientThreadFunc(UWingServer* Server, TSharedPtr<FClientConnection> Client)
|
||||
{
|
||||
constexpr int32 MaxRecvBufBytes = 1024 * 1024;
|
||||
constexpr int32 MinUnusedRecvSpace = 4096;
|
||||
|
||||
FSocket* Socket = Client->Socket;
|
||||
FString LineBuffer;
|
||||
uint8 RecvBuf[4096];
|
||||
TArray<uint8> 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<FAssetRegistryModule>("AssetRegistry").Get();
|
||||
while (AR.IsLoadingAssets()) FPlatformProcess::Sleep(0.25f);
|
||||
Client->bDone = true;
|
||||
return;
|
||||
}
|
||||
|
||||
// Enqueue the line for game-thread processing
|
||||
TSharedPtr<UWingServer::FPendingMessage> Msg = MakeShared<UWingServer::FPendingMessage>();
|
||||
Msg->Line = Line;
|
||||
TFuture<FString> 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<const uint8*>(Utf8.Get()),
|
||||
Utf8.Length() + 1))
|
||||
{
|
||||
Client->bDone = true;
|
||||
return;
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
if (!ReceiveMoreBytesIntoBuffer(Socket, RecvBuf, RecvLen))
|
||||
{
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
Client->bDone = true;
|
||||
}
|
||||
|
||||
bool UWingServer::ExtractRequestFromBuffer(
|
||||
TArray<uint8>& RecvBuf, int32& RecvLen, FString& OutRequest)
|
||||
{
|
||||
const uint8* EndOfRequest = static_cast<const uint8*>(
|
||||
memchr(RecvBuf.GetData(), '\0', RecvLen));
|
||||
if (EndOfRequest == nullptr)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
const int32 MessageLen =
|
||||
static_cast<int32>(EndOfRequest - RecvBuf.GetData());
|
||||
OutRequest = FString::ConstructFromPtrSize(
|
||||
reinterpret_cast<const UTF8CHAR*>(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<uint8>& 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<UWingServer::FPendingMessage> Msg =
|
||||
MakeShared<UWingServer::FPendingMessage>();
|
||||
Msg->Line = Request;
|
||||
TFuture<FString> 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<FAssetRegistryModule>(
|
||||
"AssetRegistry").Get();
|
||||
while (AR.IsLoadingAssets()) FPlatformProcess::Sleep(0.25f);
|
||||
}
|
||||
|
||||
// ============================================================
|
||||
// BuildWingHandlerRegistry
|
||||
// ============================================================
|
||||
|
||||
@@ -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<FWingProperty>& Props, const FJsonValue& Json,
|
||||
bool AllOptional, WingOut Errors);
|
||||
|
||||
// Functions to populate properties from a JSON object.
|
||||
//
|
||||
|
||||
private:
|
||||
static void StripEditable(TArray<FWingProperty> &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);
|
||||
};
|
||||
|
||||
@@ -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<FClientConnection> Client);
|
||||
static bool ExtractRequestFromBuffer(
|
||||
TArray<uint8>& RecvBuf, int32& RecvLen, FString& OutRequest);
|
||||
static bool ReceiveMoreBytesIntoBuffer(
|
||||
FSocket* Socket, TArray<uint8>& 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
|
||||
|
||||
@@ -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""
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user