From e108eddc22125fb64cadfe8e9f1496ecd7cadf8c Mon Sep 17 00:00:00 2001 From: Tim Moore Date: Thu, 15 Jan 2009 13:05:23 +0100 Subject: [PATCH 1/2] FGMultiplayMgr: use binary search to find a property by id --- src/MultiPlayer/multiplaymgr.cxx | 69 ++++++++++++++++++++++---------- src/MultiPlayer/multiplaymgr.hxx | 5 ++- src/Network/multiplay.cxx | 5 +-- 3 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/MultiPlayer/multiplaymgr.cxx b/src/MultiPlayer/multiplaymgr.cxx index 68ca3c245..9bbb4aaac 100644 --- a/src/MultiPlayer/multiplaymgr.cxx +++ b/src/MultiPlayer/multiplaymgr.cxx @@ -32,6 +32,7 @@ #endif #include +#include #include #include @@ -58,7 +59,7 @@ const char sMULTIPLAYMGR_HID[] = MULTIPLAYTXMGR_HID; // This should be extendable dynamically for every specific aircraft ... // For now only that static list FGMultiplayMgr::IdPropertyList -FGMultiplayMgr::sIdPropertyList[] = { +const FGMultiplayMgr::sIdPropertyList[] = { {100, "surface-positions/left-aileron-pos-norm", SGPropertyNode::FLOAT}, {101, "surface-positions/right-aileron-pos-norm", SGPropertyNode::FLOAT}, {102, "surface-positions/elevator-pos-norm", SGPropertyNode::FLOAT}, @@ -220,11 +221,47 @@ FGMultiplayMgr::sIdPropertyList[] = { {10317, "sim/multiplay/generic/int[17]", SGPropertyNode::INT}, {10318, "sim/multiplay/generic/int[18]", SGPropertyNode::INT}, {10319, "sim/multiplay/generic/int[19]", SGPropertyNode::INT}, - - /// termination - {0, 0, SGPropertyNode::UNSPECIFIED} }; +const unsigned +FGMultiplayMgr::numProperties = (sizeof(FGMultiplayMgr::sIdPropertyList) + / sizeof(FGMultiplayMgr::sIdPropertyList[0])); + +// Look up a property ID using binary search. +namespace +{ + struct ComparePropertyId + { + bool operator()(const FGMultiplayMgr::IdPropertyList& lhs, + const FGMultiplayMgr::IdPropertyList& rhs) + { + return lhs.id < rhs.id; + } + bool operator()(const FGMultiplayMgr::IdPropertyList& lhs, + unsigned id) + { + return lhs.id < id; + } + bool operator()(unsigned id, + const FGMultiplayMgr::IdPropertyList& rhs) + { + return id < rhs.id; + } + }; + +} +const FGMultiplayMgr::IdPropertyList* FGMultiplayMgr::findProperty(unsigned id) +{ + std::pair result + = std::equal_range(sIdPropertyList, sIdPropertyList + numProperties, id, + ComparePropertyId()); + if (result.first == result.second) { + return 0; + } else { + return result.first; + } +} + ////////////////////////////////////////////////////////////////////// // // MultiplayMgr constructor @@ -685,21 +722,11 @@ FGMultiplayMgr::ProcessPosMsg(const char *Msg, netAddress & SenderAddress, xdr++; // Check the ID actually exists and get the type - unsigned i = 0; - bool found = false; - while (FGMultiplayMgr::sIdPropertyList[i].name) - { - if (sIdPropertyList[i].id == pData->id) - { - found = true; - pData->type = sIdPropertyList[i].type; - } - - i++; - } + const IdPropertyList* plist = findProperty(pData->id); - if (found == true) + if (plist) { + pData->type = plist->type; // How we decode the remainder of the property depends on the type switch (pData->type) { case SGPropertyNode::INT: @@ -770,7 +797,8 @@ FGMultiplayMgr::ProcessPosMsg(const char *Msg, netAddress & SenderAddress, else { // We failed to find the property. We'll try the next packet immediately. - //cout << " Unknown\n"; + SG_LOG(SG_NETWORK, SG_WARN, "FGMultiplayMgr::ProcessPosMsg - " + << "found unknown property id" << pData->id); } } @@ -852,11 +880,8 @@ FGMultiplayMgr::addMultiplayer(const std::string& callsign, aiMgr->attach(mp); /// FIXME: that must follow the attach ATM ... - unsigned i = 0; - while (sIdPropertyList[i].name) { + for (unsigned i = 0; i < numProperties; ++i) mp->addPropertyId(sIdPropertyList[i].id, sIdPropertyList[i].name); - ++i; - } } return mp; diff --git a/src/MultiPlayer/multiplaymgr.hxx b/src/MultiPlayer/multiplaymgr.hxx index 61cd2c49c..2cca356c5 100644 --- a/src/MultiPlayer/multiplaymgr.hxx +++ b/src/MultiPlayer/multiplaymgr.hxx @@ -60,8 +60,11 @@ public: const char* name; SGPropertyNode::Type type; }; - static IdPropertyList sIdPropertyList[]; + static const IdPropertyList sIdPropertyList[]; + static const unsigned numProperties; + static const IdPropertyList* findProperty(unsigned id); + FGMultiplayMgr(); ~FGMultiplayMgr(); bool init(void); diff --git a/src/Network/multiplay.cxx b/src/Network/multiplay.cxx index 44af02ad5..cea4e8ab4 100644 --- a/src/Network/multiplay.cxx +++ b/src/Network/multiplay.cxx @@ -101,13 +101,12 @@ bool FGMultiplay::open() { SGPropertyNode* root = globals->get_props(); /// Build up the id to property map - unsigned i = 0; - while (FGMultiplayMgr::sIdPropertyList[i].name) { + + for (unsigned i = 0; i < FGMultiplayMgr::numProperties; ++i) { const char* name = FGMultiplayMgr::sIdPropertyList[i].name; SGPropertyNode* pNode = root->getNode(name); if (pNode) mPropertyMap[FGMultiplayMgr::sIdPropertyList[i].id] = pNode; - ++i; } return is_enabled(); From 45194f8e817b758116e2433b607fc894e7dd3c05 Mon Sep 17 00:00:00 2001 From: Tim Moore Date: Thu, 15 Jan 2009 16:03:34 +0100 Subject: [PATCH 2/2] Pad T_PositionMsg to a multiple of 8 bytes, and check for messages that aren't. T_PositionMsg had different sizes on 32 and 64 bit systems, which is bad when a struct is put directly into an network message. Try to work around this difference in old clients still on the network. --- src/MultiPlayer/mpmessages.hxx | 6 ++ src/MultiPlayer/multiplaymgr.cxx | 96 +++++++++++++++++++++++++++++++- 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/src/MultiPlayer/mpmessages.hxx b/src/MultiPlayer/mpmessages.hxx index 12655a0ce..2499da2cf 100644 --- a/src/MultiPlayer/mpmessages.hxx +++ b/src/MultiPlayer/mpmessages.hxx @@ -108,6 +108,12 @@ struct T_PositionMsg { // angular acceleration wrt the earth centered frame measured in // the earth centered frame xdr_data_t angularAccel[3]; + // Padding. The alignment is 8 bytes on x86_64 because there are + // 8-byte types in the message, so the size should be explicitly + // rounded out to a multiple of 8. Of course, it's a bad idea to + // put a C struct directly on the wire, but that's a fight for + // another day... + xdr_data_t pad; }; struct FGPropertyData { diff --git a/src/MultiPlayer/multiplaymgr.cxx b/src/MultiPlayer/multiplaymgr.cxx index 9bbb4aaac..2188129a1 100644 --- a/src/MultiPlayer/multiplaymgr.cxx +++ b/src/MultiPlayer/multiplaymgr.cxx @@ -33,6 +33,7 @@ #include #include +#include // isNaN #include #include @@ -262,6 +263,74 @@ const FGMultiplayMgr::IdPropertyList* FGMultiplayMgr::findProperty(unsigned id) } } +namespace +{ + bool verifyProperties(const xdr_data_t* data, const xdr_data_t* end) + { + const xdr_data_t* xdr = data; + while (xdr < end) { + unsigned id = XDR_decode_uint32(*xdr); + const FGMultiplayMgr::IdPropertyList* plist + = FGMultiplayMgr::findProperty(id); + + if (plist) { + xdr++; + // How we decode the remainder of the property depends on the type + switch (plist->type) { + case SGPropertyNode::INT: + case SGPropertyNode::BOOL: + case SGPropertyNode::LONG: + xdr++; + break; + case SGPropertyNode::FLOAT: + case SGPropertyNode::DOUBLE: + { + float val = XDR_decode_float(*xdr); + if (osg::isNaN(val)) + return false; + xdr++; + break; + } + case SGPropertyNode::STRING: + case SGPropertyNode::UNSPECIFIED: + { + // String is complicated. It consists of + // The length of the string + // The string itself + // Padding to the nearest 4-bytes. + // XXX Yes, each byte is padded out to a word! Too late + // to change... + uint32_t length = XDR_decode_uint32(*xdr); + xdr++; + if ((length > 0) && (length < MAX_TEXT_SIZE)) { + xdr += length; + // Now handle the padding + while ((length % 4) != 0) + { + xdr++; + length++; + //cout << "0"; + } + } else { + // The string appears to be invalid; bail. + return false; + } + } + break; + default: + // cerr << "Unknown Prop type " << id << " " << type << "\n"; + xdr++; + break; + } + } + else { + // give up; this is a malformed property list. + return false; + } + } + return true; + } +} ////////////////////////////////////////////////////////////////////// // // MultiplayMgr constructor @@ -390,6 +459,8 @@ FGMultiplayMgr::SendMyPosition(const FGExternalMotionData& motionInfo) } T_PositionMsg PosMsg; + + memset(&PosMsg, 0, sizeof(PosMsg)); strncpy(PosMsg.Model, fgGetString("/sim/model/path"), MAX_MODEL_NAME_LEN); PosMsg.Model[MAX_MODEL_NAME_LEN - 1] = '\0'; @@ -712,6 +783,29 @@ FGMultiplayMgr::ProcessPosMsg(const char *Msg, netAddress & SenderAddress, //cout << "INPUT MESSAGE\n"; xdr_data_t* xdr = (xdr_data_t*) (Msg + sizeof(T_MsgHdr) + sizeof(T_PositionMsg)); + // There was a bug in 1.9.0 and before: T_PositionMsg was 196 bytes + // on 32 bit architectures and 200 bytes on 64 bit, and this + // structure is put directly on the wire. By looking at the padding, + // we can sort through the mess, mostly: + // If padding is 0 (which is not a valid property type), then the + // message was produced by a new client or an old 64 bit client that + // happened to have 0 on the stack; + // Else if the property list starting with the padding word is + // well-formed, then the client is probably an old 32 bit client and + // we'll go with that; + // Else it is an old 64-bit client and properties start after the + // padding. + // There is a chance that we could be fooled by garbage in the + // padding looking like a valid property, so verifyProperties() is + // strict about the validity of the property values. + if (PosMsg->pad != 0) { + if (verifyProperties(&PosMsg->pad, + reinterpret_cast(Msg + len))) + xdr = &PosMsg->pad; + else if (!verifyProperties(xdr, + reinterpret_cast(Msg + len))) + goto noprops; + } while ((char*)xdr < Msg + len) { FGPropertyData* pData = new FGPropertyData; SGPropertyNode::Type type = SGPropertyNode::UNSPECIFIED; @@ -801,7 +895,7 @@ FGMultiplayMgr::ProcessPosMsg(const char *Msg, netAddress & SenderAddress, << "found unknown property id" << pData->id); } } - + noprops: FGAIMultiplayer* mp = getMultiplayer(MsgHdr->Callsign); if (!mp) mp = addMultiplayer(MsgHdr->Callsign, PosMsg->Model);