Thursday, March 4, 2010

A bugreport for XOAD

I realize that XOAD is a dead project and all, but we still have a website around that uses it, and which, in turn, is used by a fair number of people. This website recently broke, probably due to an upgrade in the Apache PHP module (to v. 5.2). This cost me a few sleepless nights, but I also learned a bunch in the process.

The symptom we were seeing was an unserialization error. Basically, XOAD has its own code for serializing Javascript objects into PHP format. The server-side then calls PHP's unserialize() on them. Some of those serialized objects were not unserializing properly. Specifically, objects with deeply nested __meta fields were failing to unserialize. It took days to figure out what the hell was going on, mainly because I've never looked at AJAX before in my life, having spent the last 8 years hacking numerical code in C. In the end it was instrumentation of the offending XOAD code with error_log()s that solved it. And it turned out that the bug was due to human error, a human error that's gone undetected FIVE years!

The error is in classes/Client.class.php in the XOAD_Client::register() function. This function builds objects that are then passed on to the serializer. The line that reads:
$attachMeta[$key] = $valueType;
should be:
$attachMeta[$key] = $value; 

That's it. Isn't it reminiscent of those sentences that completely change meaning when you move around their commas?

So why did this bug show up now? I only have a guess as to the answer. The effect of this bug was that in objects with deep (as in 2 levels down, don't imagine anything extreme) __meta attributes, some of the fields that should have been filled with the values of variables were filled with their types (duh). From a semantic point of view, this did not affect our application, because we never looked into those deep levels of the objects in question. But what about from the point of view of serialization? When the field for which we get the type instead of contents is __size, and __size is then used during serialization in a meaningful way, then there's the potential for problems. Consider this fragment of a correctly serialized object:
"__meta";O:6:"string":7:{s:1:"0";N;s:1:"1";N;s:1:"2";N;s:1:"3";N;s:1:"4";N;s:1:"5";N;s:1:"6";N;}
vs the same fragment serialized incorrectly, with the size field (derived from the __size attribute of the object being serialized) replaced with "int":
"__meta";O:6:"string":int:{s:1:"0";N;s:1:"1";N;s:1:"2";N;s:1:"3";N;s:1:"4";N;s:1:"5";N;s:1:"6";N;}

I suspect that our previous version of the Apache PHP module contained an unserialize() function that unserialized on the basis of curly brackets only. Based on curly bracket matching, the second fragment will unserialize just fine. But if the newer Apache PHP module actually looks at the size field, which precedes the contents of the array in curly brackets, then the first fragment will unserialize, whereas the second won't ("int" not being a valid array size). Is the second, stricter unserializing method the correct one? Absolutely! I don't know that this is what happened, but if I'm right, I can deduce that PHP has improved. I can get behind that.

1 comments:

  1. Thank you for the bug report, Terroar. I will try and find some time over the weekend to add your bug fix to the release. It has been a while since I looked at XOAD, but I am glad to see there are still some real-world projects around that make use of it. Let me know if you ran into any other problems and if you have patches for them.

    I plan to move XOAD over to GitHub soon, hopefully more people can join and exchange ideas/improvements on the legacy code.

    All the best,
    Stan (original XOAD author)

    ReplyDelete