Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

avoid crash due to message overflow via data truncation, adds message limit #defines, increases default buffer size #270

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

avoid crash due to message overflow via data truncation, adds message limit #defines, increases default buffer size #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ ipch/
*.user
*.sdf
*.sbr
.vs/

# XCode Files #
###############
Expand Down
10 changes: 5 additions & 5 deletions xpcPlugin/DataManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ namespace XPC
static map<DREF, XPLMDataRef> mdrefs[PLANE_COUNT];
static map<string, XPLMDataRef> sdrefs;

DREF XPData[134][8] = { DREF_None };
DREF XPData[XPC_MAX_COLS][8] = { DREF_None };

void DataManager::Initialize()
{
Expand Down Expand Up @@ -357,7 +357,7 @@ namespace XPC
}
if ((dataType & 16) == 16) // Integer array
{
const std::size_t TMP_SIZE = 200;
const std::size_t TMP_SIZE = XPC_MAX_DREF_VALUES;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unclear why this was limited to 200 when protocol is set to return up to 255

int iValues[TMP_SIZE];
int drefSize = XPLMGetDatavi(xdref, NULL, 0, 0);
if (drefSize > size)
Expand All @@ -382,7 +382,7 @@ namespace XPC
}
if ((dataType & 32) == 32) // Byte array
{
const std::size_t TMP_SIZE = 1024;
const std::size_t TMP_SIZE = XPC_MAX_DREF_VALUES;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strange to have a different cap of 1024 here, since requests ask for max size of 255 values (so count fits in one byte), so even if we find 1024 values we then discard down to 255 anyway

char bValues[TMP_SIZE];
int drefSize = XPLMGetDatab(xdref, NULL, 0, 0);
if (drefSize > size)
Expand Down Expand Up @@ -561,7 +561,7 @@ namespace XPC
}
else if ((dataType & 16) == 16) // Integer Array
{
const std::size_t TMP_SIZE = 200;
const std::size_t TMP_SIZE = XPC_MAX_DREF_VALUES;
int iValues[TMP_SIZE];
int drefSize = XPLMGetDatavi(xdref, NULL, 0, 0);
if (size > drefSize)
Expand All @@ -587,7 +587,7 @@ namespace XPC
}
else if ((dataType & 32) == 32) // Byte Array
{
const std::size_t TMP_SIZE = 1024;
const std::size_t TMP_SIZE = XPC_MAX_DREF_VALUES;
char bValues[TMP_SIZE];
int drefSize = XPLMGetDatab(xdref, NULL, 0, 0);
if (size > drefSize)
Expand Down
3 changes: 2 additions & 1 deletion xpcPlugin/DataManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#define XPCPLUGIN_DATAMANAGER_H_

#include <string>
#include "XPCLimits.h"

namespace XPC
{
Expand Down Expand Up @@ -137,7 +138,7 @@ namespace XPC
};

/// Maps X-Plane dataref lines to XPC DREF values.
extern DREF XPData[134][8];
extern DREF XPData[XPC_MAX_COLS][8];

/// Contains methods to martial data between the plugin and X-Plane.
///
Expand Down
3 changes: 2 additions & 1 deletion xpcPlugin/Message.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#define XPCPLUGIN_MESSAGE_H_

#include "UDPSocket.h"
#include "XPCLimits.h"

namespace XPC
{
Expand Down Expand Up @@ -44,7 +45,7 @@ namespace XPC
private:
Message();

static const std::size_t bufferSize = 4096;
static const std::size_t bufferSize = XPC_MAX_MESSAGE_SIZE;
unsigned char buffer[bufferSize];
std::size_t size;
struct sockaddr source;
Expand Down
25 changes: 16 additions & 9 deletions xpcPlugin/MessageHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,12 +298,12 @@ namespace XPC
return;
}

if (numCols > 134) // Error. Will overflow values
if (numCols > XPC_MAX_COLS) // Error. Will overflow values
{
Log::FormatLine(LOG_ERROR, "DATA", "ERROR: numCols to large.");
return;
}
float values[134][9];
float values[XPC_MAX_COLS][9];
for (int i = 0; i < numCols; ++i)
{
// 5 byte header + (9 * 4 = 36) bytes per row
Expand All @@ -318,9 +318,9 @@ namespace XPC
for (int i = 0; i < numCols; ++i)
{
unsigned char dataRef = (unsigned char)values[i][0];
if (dataRef >= 134)
if (dataRef >= XPC_MAX_COLS)
{
Log::FormatLine(LOG_ERROR, "DATA", "ERROR: DataRef # must be between 0 - 134 (Received: %hi)", (int)dataRef);
Log::FormatLine(LOG_ERROR, "DATA", "ERROR: DataRef # must be between 0 - %hi (Received: %hi)", (int)XPC_MAX_COLS, (int)dataRef);
continue;
}

Expand Down Expand Up @@ -533,16 +533,23 @@ namespace XPC
connections[connectionKey] = connection;
}

unsigned char response[4096] = "RESP";
unsigned char response[XPC_MAX_MESSAGE_SIZE] = "RESP";
response[5] = drefCount;
std::size_t cur = 6;
for (int i = 0; i < drefCount; ++i)
{
float values[255];
int count = DataManager::Get(connection.getdRequest[i], values, 255);
float values[XPC_MAX_DREF_VALUES];
int count = DataManager::Get(connection.getdRequest[i], values, XPC_MAX_DREF_VALUES);
int dataSize = count * sizeof(float);
if (cur + drefCount - i + dataSize > XPC_MAX_MESSAGE_SIZE) {
Log::FormatLine(LOG_ERROR, "GETD", "ERROR: omitting data (%i bytes) for dref %i to avoid exceeding message buffer for connection %i.",
dataSize, i, connection.id);
count = 0;
dataSize = 0;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this omits data if it would prevent writing at least a single count byte for all remaining drefs. this will always succeed as long as the message size is larger than the max allowed dref count, e.g. XPC_MAX_MESSAGE_SIZE > 255 + message header.

if there's concern about some clients not dealing with empty value arrays (I only tested the python client where it's fine), this could be changed to return only a single value from the array if there's insufficient space to write 1 byte + 1 float for all remaining drefs.

response[cur++] = count;
memcpy(response + cur, values, count * sizeof(float));
cur += count * sizeof(float);
if (dataSize) memcpy(response + cur, values, dataSize);
cur += dataSize;
}

sock->SendTo(response, cur, &connection.addr);
Expand Down
2 changes: 1 addition & 1 deletion xpcPlugin/MessageHandlers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ namespace XPC
unsigned char id;
sockaddr addr;
unsigned char getdCount;
std::string getdRequest[255];
std::string getdRequest[XPC_MAX_DREF_COUNT];
} ConnectionInfo;

static std::map<std::string, ConnectionInfo> connections;
Expand Down
4 changes: 4 additions & 0 deletions xpcPlugin/XPCLimits.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#define XPC_MAX_MESSAGE_SIZE 16384 // Max message size in Message.h; was 4096
#define XPC_MAX_DREF_VALUES 255 // Max dref elements, must fit in single byte
#define XPC_MAX_DREF_COUNT 255 // Max number of drefs requested per message, encoded as single byte
#define XPC_MAX_COLS 134 // Max number of columns in data array, MessageHandlers