T128029 TemplateData's PHP JSON validation isn't strict enough because WMF cluster's HHVM allows trailing commas (original) (raw)

TemplateData's PHP JSON validation isn't strict enough because WMF cluster's HHVM allows trailing commas

Closed, ResolvedPublic1 Estimated Story Points

• Edit Task
• Edit Related Tasks...
• Create Subtask
• Edit Parent Tasks
• Edit Subtasks
• Merge Duplicates In
• Close As Duplicate
• Edit Related Objects...
• Edit Commits
• Edit Mocks
• Mute Notifications
• Protect as security issue

Commas at the end of object definitions aren't allowed, and are rejected on ingestion by the TD GUI but not on save by the TD PHP when running on HHVM without this compile flag (Zend runs it fine).

• Mentions
• Duplicates

Event Timeline

Jdforrester-WMF renamed this task from TemplateData's PHP JSON validation isn't strict enough to TemplateData's PHP JSON validation isn't strict enough because WMF cluster's HHVM allows trailing commas.Feb 25 2016, 12:39 AM

Comment Actions

My gut feeling on this is that everything in MW is already supposed to be UTF-8, and invalid UTF-8 already causes exceptions etc, so not tolerating it in json_decode() should be OK. In fact, I don't think Zend PHP tolerates invalid UTF-8 in JSON either, pretty sure we've already encountered that before.

Comment Actions

The reason for having two JSON stacks has its roots in a now-infamous clause in the JSON license: "The Software shall be used for Good, not Evil." This makes the license non-free in the eyes of Google, GNU, Fedora, Debian, and Red Hat Legal. Douglas Crockford is aware of the amount of pain this has caused and he doesn't care. It's some kind of license griefing. There's a section about this issue on en:Douglas Crockford.

This was not an issue for Facebook, but it was an issue for Debian. Facebook did not want to switch something as fundamental as the JSON parsing code in their core application runtime, so the compromise was to use either JSON or JSON-C, based on a compile-time flag. The person who actually implemented this is none other than our very own @EBernhardson.

The bit (literally!) that seems to be causing us problem is the commented-out code on lines 225-227 of hphp/runtime/ext/json/jsonc_parser.cpp:

//if (!(options & k_JSON_FB_LOOSE)) { // json_tokener_set_flags(tok, JSON_TOKENER_STRICT); //}

What this code does (or rather: would be doing, if it were not commented-out) is set the JSON-C JSON_TOKENER_STRICT flag unless the non-standard JSON_FB_LOOSE flag was set in the $options parameter for json_decode(). From this snippet, I gather that JSON-C's default behavior is nonstrict, which is what we're seeing.

Why is this code commented out? I don't know (or don't remember). Maybe Erik can shed some light.

Comment Actions

By the way, it's not just HHVM that suffers from this licensing nightmare; see PHP bug 63520 (watch out for guest appearances by some MediaWiki personages). The third comment from the bottom talks about some of the bugs that this has caused.

Comment Actions

I took a look back over the code I submitted but I can't remember right now why that bit of code was commented out. It might have been to do with passing the existing test cases, but that's just a guess. In the 2 years it seems to have fallen out of my head...

Comment Actions

My guess is that you were considering the possibility of replacing JSON with JSON-C entirely, by checking how closely JSON-C's behavior in non-strict mode matched JSON w/JSON_FB_LOOSE modifications.

At any rate, I built HHVM with that code un-commented, and saw that indeed it made json_decode() strict on trailing commas and single-quotes, matching Zend's behavior. And all the tests pass. So, unless you think there is a reason not to, we could simply submit that upstream.

Comment Actions

This is a dupe of T107132, but since this task is currently active I'm merging T107132 into this task rather than the other way around.

Comment Actions

As a temporary workaround, you may be able to do something like this: