-
Notifications
You must be signed in to change notification settings - Fork 277
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.
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
avoid crash due to message overflow via data truncation, adds message limit #defines, increases default buffer size #270
base: master
Are you sure you want to change the base?
Conversation
@@ -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; |
There was a problem hiding this comment.
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
@@ -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; |
There was a problem hiding this comment.
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
dataSize, i, connection.id); | ||
count = 0; | ||
dataSize = 0; | ||
} |
There was a problem hiding this comment.
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.
Summary
This pull request addresses issue 226 where requesting too many datarefs and/or datarefs with large numbers of values could overflow the fixed message buffer of 4096 bytes and cause XPlane to crash to the desktop.
This PR extends the idea in PR266 but rather than simply aborting the message response completely, a dataref that would overflow the buffer is replaced with an empty list (a value count of 0 and no data), allowing a valid response to the client. In those cases, the client will see an empty list for the offending dataref(s) rather than the expected dataref content, and an error message will be logged in
XPCLog.txt
like:This PR also increases the default message buffer from 4096 to 16384 bytes (which is still safe for a UDP packet, and the max message size requested by the Python client), and replaces several other numeric constants repeated throughout the code with #define'd constant values via
XPCLimits.h
Testing
Tested via python client by requesting all 4600+ datarefs listed in
Resources/plugins/DataRefs.txt
in blocks of 250. No segfault was observed, and drefs that previously would have overflowed are successfully parsed as an empty list by the Python client, with expected error messages logged inXPCLog.txt
.