CodeSOD: A Stack of XML

Alice works with an XML-based RPC system. They send requests, it does a thing, and returns back a response, all surrounded in XML. The service sending the requests, however, doesn’t use a well-established parsing library. The result is that, well, the code ends up here.

First, though, a bit about the data they expect to parse. It’s verbose, but fairly straightforward:

<methodResponse>
<params>
<param>
<value>
<struct>
<member>
<name>foo</name>
<value>
<string>abcde</string>
</value>
</member>
<member>
<name>bar</name>
<value>
<int>12345</int>
</value>
</member>
</struct>
</value>
</param>
</params>
</methodResponse>

The tree is awkward, but the response contains params, where each param may contain a struct, the struct may contain members, which have a name and a value, where the value contains the actual data, wrapped in a type.

But there’s one thing missing here: the service which sends this response recently changed its API, and for some fields, includes an empty <data/> tag, instead of a value. This particular client doesn’t care about data fields, and should be able to safely ignore them if they exist.

Now, you might think, “well, that should be easy then,” and you’d be right if you were just using a generic XML parser. They didn’t do that. They hand-rolled a parser that is specific to this data format. So let’s see what happened:

void XmlRpcResponseParser::OnStartElement(const XML_Char *pszName, const XML_Char **papszAttrs)
{
const string element = pszName;
// Sanity
if (_states.empty()) {
if (element != “methodResponse” && !_get_method_response) {
throw “Invalid top-level element <“ + element + “> expecting <methodResponse>”;
}

_got_method_response = true;
_states.push(&response);
}

if (element == “fault”) {
// response is faulty: just extract error’s code and description
is_fault = true;
} else if (!is_fault && element == “struct”) {
top()->type = XML_RPC_STRUCT;
} else if (!is_fault && element == “array”) {
top()->type = XML_RPC_ARRAY;
} else if (!is_fault && (element == “data” || element == “value”)) {
if (top()->type == XML_RPC_ARRAY) {
top()->xml_array.push_back(Entry());
_states.push(&top()->xml_array.back()); // <———— #1
}
}

_cdata.clear();
}

Here, they track a stack, so they can keep track of where they are in parsing. Oh, except they don’t. They only push onto the stack when they encounter a data or a value element. When the response only contained values, this worked fine. So for years, it sat like this.

But when they added data tags, it stopped working, specifically because of how they handle closing tags:

void XmlRpcResponseParser::OnEndElement(const XML_Char *pszName)
{
const string element = pszName;

if (element == “int” || element == “i4”) {
top()->integer = atoi(chomp(_cdata).c_str());
top()->type = XML_RPC_INTEGER;
} else if (element == “string”) {
top()->str = _cdata;
top()->type = XML_RPC_STRING;
} else if (element == “boolean”) {
const string value = chomp(_cdata);
top()->boolean = (value == “1” || value == “true”);
top()->type = XML_RPC_BOOLEAN;
} else if (!is_fault && element == “name”) {
_states.push(&top()->xml_struct[_cdata]);
} else if (!is_fault && element == “value”) {
if (!_states.empty()) {
_states.pop(); // <————- #2
}
}

_cdata.clear();
}

Note here that they only pop when they encounter a value element. Which means when they encounter a data element, they push the stack, but never pop it, which gets the whole tree desynced and breaks parsing.

Since this was discovered, most of the service calls have been migrated to use JSON instead of XML. That “solves” the problem, given that the XML parser is broken. But the XML parser is still used for some calls, and the result is that the service being invoked is constrained in how it’s allowed to change its API- it can’t add data fields to certain responses, because this client will break. Everyone hates this, and someday, the XML endpoints will go away. Someday.

Source: Read More